From e8ff354713f611f333fad8be78c6d05d3ad705b4 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Tue, 26 Oct 2021 14:03:19 +1100 Subject: [PATCH 1/9] feat(ext/fetch): support fetching local files Closes #11925 Closes #2150 --- cli/tests/unit/fetch_test.ts | 58 +++++++++++++++++++++++++++++++++++- ext/fetch/lib.rs | 30 +++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/cli/tests/unit/fetch_test.ts b/cli/tests/unit/fetch_test.ts index bc61d67b52da38..98134728e43940 100644 --- a/cli/tests/unit/fetch_test.ts +++ b/cli/tests/unit/fetch_test.ts @@ -23,7 +23,7 @@ unitTest( unitTest({ permissions: { net: true } }, async function fetchProtocolError() { await assertRejects( async () => { - await fetch("file:///"); + await fetch("ftp://localhost:21/a/file"); }, TypeError, "not supported", @@ -1360,3 +1360,59 @@ unitTest( client.close(); }, ); + +unitTest(async function fetchFilePerm() { + await assertRejects(async () => { + await fetch(new URL("../testdata/subdir/json_1.json", import.meta.url)); + }, Deno.errors.PermissionDenied); +}); + +unitTest(async function fetchFilePermDoesNotExist() { + await assertRejects(async () => { + await fetch(new URL("./bad.json", import.meta.url)); + }, Deno.errors.PermissionDenied); +}); + +unitTest( + { permissions: { read: true } }, + async function fetchFileBadMethod() { + await assertRejects( + async () => { + await fetch( + new URL("../testdata/subdir/json_1.json", import.meta.url), + { + method: "POST", + }, + ); + }, + TypeError, + "Fetching files only supports the GET method. Received POST.", + ); + }, +); + +unitTest( + { permissions: { read: true } }, + async function fetchFileDoesNotExist() { + await assertRejects( + async () => { + await fetch(new URL("./bad.json", import.meta.url)); + }, + TypeError, + ); + }, +); + +unitTest( + { permissions: { read: true } }, + async function fetchFile() { + const res = await fetch( + new URL("../testdata/subdir/json_1.json", import.meta.url), + ); + assert(res.ok); + const fixture = await Deno.readTextFile( + "cli/tests/testdata/subdir/json_1.json", + ); + assertEquals(await res.text(), fixture); + }, +); diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index 13adae1a761ac5..1744eef172a692 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -167,6 +167,36 @@ where // Check scheme before asking for net permission let scheme = url.scheme(); let (request_rid, request_body_rid, cancel_handle_rid) = match scheme { + "file" => { + let permissions = state.borrow_mut::(); + let path = url + .to_file_path() + .map_err(|_| type_error(format!("Invalid file URL: {}", url)))?; + permissions.check_read(&path)?; + if method != Method::GET { + return Err(type_error(format!( + "Fetching files only supports the GET method. Received {}.", + method + ))); + } + if !path.is_file() { + return Err(type_error(format!("Unable to fetch \"{}\".", url))); + } + + let body = std::fs::read(&path)?; + + let response = http::Response::builder() + .status(http::StatusCode::OK) + .body(reqwest::Body::from(body))?; + + let fut = async move { Ok(Ok(Response::from(response))) }; + + let request_rid = state + .resource_table + .add(FetchRequestResource(Box::pin(fut))); + + (request_rid, None, None) + } "http" | "https" => { let permissions = state.borrow_mut::(); permissions.check_net_url(&url)?; From 84191398a6dabce53f66d4a10ae745674d079d06 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Thu, 28 Oct 2021 12:28:14 +1100 Subject: [PATCH 2/9] address feedback --- ext/fetch/lib.rs | 36 ++++++++++++++++++++++++++---------- runtime/build.rs | 1 + runtime/web_worker.rs | 1 + runtime/worker.rs | 1 + 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index 1744eef172a692..42effbb91bcae3 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -4,6 +4,7 @@ use data_url::DataUrl; use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::futures::Future; +use deno_core::futures::FutureExt; use deno_core::futures::Stream; use deno_core::futures::StreamExt; use deno_core::include_js_files; @@ -59,6 +60,7 @@ pub fn init( request_builder_hook: Option RequestBuilder>, unsafely_ignore_certificate_errors: Option>, client_cert_chain_and_key: Option<(String, String)>, + enable_file_fetch: bool, ) -> Extension { Extension::builder() .js(include_js_files!( @@ -103,6 +105,7 @@ pub fn init( .clone(), client_cert_chain_and_key: client_cert_chain_and_key.clone(), }); + state.put::(FetchFeatures { enable_file_fetch }); Ok(()) }) .build() @@ -117,6 +120,10 @@ pub struct HttpClientDefaults { pub client_cert_chain_and_key: Option<(String, String)>, } +struct FetchFeatures { + enable_file_fetch: bool, +} + pub trait FetchPermissions { fn check_net_url(&mut self, _url: &Url) -> Result<(), AnyError>; fn check_read(&mut self, _p: &Path) -> Result<(), AnyError>; @@ -160,6 +167,7 @@ where let client = state.borrow::(); client.clone() }; + let features = state.borrow::(); let method = Method::from_bytes(&args.method)?; let url = Url::parse(&args.url)?; @@ -167,7 +175,7 @@ where // Check scheme before asking for net permission let scheme = url.scheme(); let (request_rid, request_body_rid, cancel_handle_rid) = match scheme { - "file" => { + "file" if features.enable_file_fetch => { let permissions = state.borrow_mut::(); let path = url .to_file_path() @@ -183,17 +191,25 @@ where return Err(type_error(format!("Unable to fetch \"{}\".", url))); } - let body = std::fs::read(&path)?; - - let response = http::Response::builder() - .status(http::StatusCode::OK) - .body(reqwest::Body::from(body))?; + let fut = async move { + let response = { + match tokio::fs::read(&path).await { + Ok(body) => match http::Response::builder() + .status(http::StatusCode::OK) + .body(reqwest::Body::from(body)) + { + Ok(response) => Ok(Response::from(response)), + Err(err) => Err(err.into()), + }, + Err(err) => Err(err.into()), + } + }; - let fut = async move { Ok(Ok(Response::from(response))) }; + Ok(response) + } + .boxed_local(); - let request_rid = state - .resource_table - .add(FetchRequestResource(Box::pin(fut))); + let request_rid = state.resource_table.add(FetchRequestResource(fut)); (request_rid, None, None) } diff --git a/runtime/build.rs b/runtime/build.rs index b1d4fa8cbaf657..d32f72c9ff70a7 100644 --- a/runtime/build.rs +++ b/runtime/build.rs @@ -128,6 +128,7 @@ mod not_docs { None, None, None, + false, // No enable_file_fetch ), deno_websocket::init::("".to_owned(), None, None), deno_webstorage::init(None), diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs index 31fc30fbc4524d..797fde1443f0f9 100644 --- a/runtime/web_worker.rs +++ b/runtime/web_worker.rs @@ -324,6 +324,7 @@ impl WebWorker { None, options.unsafely_ignore_certificate_errors.clone(), None, + true, // enable_file_fetch ), deno_websocket::init::( options.user_agent.clone(), diff --git a/runtime/worker.rs b/runtime/worker.rs index af4095b7d2df2a..9181622a92a612 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -108,6 +108,7 @@ impl MainWorker { None, options.unsafely_ignore_certificate_errors.clone(), None, + true, // enabled_file_fetch ), deno_websocket::init::( options.user_agent.clone(), From b02f774a5b404fd877996396af87fd067cfaa5a0 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Fri, 29 Oct 2021 10:28:13 +1100 Subject: [PATCH 3/9] refactor to trait --- ext/fetch/fs_fetch_handler.rs | 61 ++++++++++++++++++++++ ext/fetch/lib.rs | 98 ++++++++++++++++++++++------------- runtime/build.rs | 4 +- runtime/web_worker.rs | 4 +- runtime/worker.rs | 4 +- 5 files changed, 129 insertions(+), 42 deletions(-) create mode 100644 ext/fetch/fs_fetch_handler.rs diff --git a/ext/fetch/fs_fetch_handler.rs b/ext/fetch/fs_fetch_handler.rs new file mode 100644 index 00000000000000..89d5ce79f0a03d --- /dev/null +++ b/ext/fetch/fs_fetch_handler.rs @@ -0,0 +1,61 @@ +// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. + +use crate::CancelHandle; +use crate::CancelableResponseFuture; +use crate::FetchHandler; +use crate::FetchRequestBodyResource; + +use deno_core::error::type_error; +use deno_core::error::AnyError; +use deno_core::futures::FutureExt; +use deno_core::url::Url; +use std::rc::Rc; + +/// An implementation which tries to read file URLs from the file system via +/// tokio::fs +#[derive(Clone)] +pub struct FsFetchHandler; + +impl FetchHandler for FsFetchHandler { + fn fetch_url( + &mut self, + url: Url, + ) -> ( + CancelableResponseFuture, + Option, + Option>, + ) { + let path = url.to_file_path().unwrap(); + let response_fut = async move { + let response = { + match tokio::fs::read(&path).await { + Ok(body) => match http::Response::builder() + .status(http::StatusCode::OK) + .body(reqwest::Body::from(body)) + { + Ok(response) => Ok(reqwest::Response::from(response)), + Err(err) => Err(err.into()), + }, + Err(err) => Err(err.into()), + } + }; + Ok(response) + } + .boxed_local(); + + (response_fut, None, None) + } + + fn validate_url(&mut self, url: &Url) -> Result<(), AnyError> { + // Error messages are kept intentionally generic in order to discourage + // probing, and attempting to discern something from the environment. + let path = url + .to_file_path() + .map_err(|_| type_error(format!("Unable to fetch \"{}\".", url)))?; + if !path.is_file() { + Err(type_error(format!("Unable to fetch \"{}\".", url))) + } else { + Ok(()) + } + } +} diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index 42effbb91bcae3..d57a97e147cb2a 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -1,10 +1,11 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. +mod fs_fetch_handler; + use data_url::DataUrl; use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::futures::Future; -use deno_core::futures::FutureExt; use deno_core::futures::Stream; use deno_core::futures::StreamExt; use deno_core::include_js_files; @@ -53,15 +54,21 @@ use tokio_util::io::StreamReader; pub use data_url; pub use reqwest; -pub fn init( +pub use fs_fetch_handler::FsFetchHandler; + +pub fn init( user_agent: String, root_cert_store: Option, proxy: Option, request_builder_hook: Option RequestBuilder>, unsafely_ignore_certificate_errors: Option>, client_cert_chain_and_key: Option<(String, String)>, - enable_file_fetch: bool, -) -> Extension { + file_fetch_handler: FH, +) -> Extension +where + FP: FetchPermissions + 'static, + FH: FetchHandler + 'static, +{ Extension::builder() .js(include_js_files!( prefix "deno:ext/fetch", @@ -75,13 +82,13 @@ pub fn init( "26_fetch.js", )) .ops(vec![ - ("op_fetch", op_sync(op_fetch::

)), + ("op_fetch", op_sync(op_fetch::)), ("op_fetch_send", op_async(op_fetch_send)), ("op_fetch_request_write", op_async(op_fetch_request_write)), ("op_fetch_response_read", op_async(op_fetch_response_read)), ( "op_fetch_custom_client", - op_sync(op_fetch_custom_client::

), + op_sync(op_fetch_custom_client::), ), ]) .state(move |state| { @@ -105,7 +112,7 @@ pub fn init( .clone(), client_cert_chain_and_key: client_cert_chain_and_key.clone(), }); - state.put::(FetchFeatures { enable_file_fetch }); + state.put::(file_fetch_handler.clone()); Ok(()) }) .build() @@ -120,10 +127,40 @@ pub struct HttpClientDefaults { pub client_cert_chain_and_key: Option<(String, String)>, } -struct FetchFeatures { - enable_file_fetch: bool, +pub type CancelableResponseFuture = + Pin>>; + +pub trait FetchHandler: Clone { + // Return the result of the fetch request consisting of a tuple of the + // cancelable response result, the optional fetch body resource and the + // optional cancel handle. + fn fetch_url( + &mut self, + url: Url, + ) -> ( + CancelableResponseFuture, + Option, + Option>, + ) { + let fut = async move { + Ok(Err(type_error(format!("Unable to fetch \"{}\".", url)))) + }; + (Box::pin(fut), None, None) + } + + // Determine if a given URL is valid, returning an early error to the client + fn validate_url(&mut self, url: &Url) -> Result<(), AnyError> { + Err(type_error(format!("Unable to fetch \"{}\".", url))) + } } +/// A default implementation which leverages the trait's default method +/// implementations. +#[derive(Clone)] +pub struct DefaultFileFetchHandler; + +impl FetchHandler for DefaultFileFetchHandler {} + pub trait FetchPermissions { fn check_net_url(&mut self, _url: &Url) -> Result<(), AnyError>; fn check_read(&mut self, _p: &Path) -> Result<(), AnyError>; @@ -152,13 +189,14 @@ pub struct FetchReturn { cancel_handle_rid: Option, } -pub fn op_fetch( +pub fn op_fetch( state: &mut OpState, args: FetchArgs, data: Option, ) -> Result where FP: FetchPermissions + 'static, + FH: FetchHandler + 'static, { let client = if let Some(rid) = args.client_rid { let r = state.resource_table.get::(rid)?; @@ -167,7 +205,6 @@ where let client = state.borrow::(); client.clone() }; - let features = state.borrow::(); let method = Method::from_bytes(&args.method)?; let url = Url::parse(&args.url)?; @@ -175,43 +212,32 @@ where // Check scheme before asking for net permission let scheme = url.scheme(); let (request_rid, request_body_rid, cancel_handle_rid) = match scheme { - "file" if features.enable_file_fetch => { - let permissions = state.borrow_mut::(); + "file" => { let path = url .to_file_path() .map_err(|_| type_error(format!("Invalid file URL: {}", url)))?; + let permissions = state.borrow_mut::(); permissions.check_read(&path)?; + if method != Method::GET { return Err(type_error(format!( "Fetching files only supports the GET method. Received {}.", method ))); } - if !path.is_file() { - return Err(type_error(format!("Unable to fetch \"{}\".", url))); - } - let fut = async move { - let response = { - match tokio::fs::read(&path).await { - Ok(body) => match http::Response::builder() - .status(http::StatusCode::OK) - .body(reqwest::Body::from(body)) - { - Ok(response) => Ok(Response::from(response)), - Err(err) => Err(err.into()), - }, - Err(err) => Err(err.into()), - } - }; - - Ok(response) - } - .boxed_local(); + let file_fetch_handler = state.borrow_mut::(); + file_fetch_handler.validate_url(&url)?; - let request_rid = state.resource_table.add(FetchRequestResource(fut)); + let (request, maybe_request_body, maybe_cancel_handle) = + file_fetch_handler.fetch_url(url); + let request_rid = state.resource_table.add(FetchRequestResource(request)); + let maybe_request_body_rid = + maybe_request_body.map(|r| state.resource_table.add(r)); + let maybe_cancel_handle_rid = maybe_cancel_handle + .map(|ch| state.resource_table.add(FetchCancelHandle(ch))); - (request_rid, None, None) + (request_rid, maybe_request_body_rid, maybe_cancel_handle_rid) } "http" | "https" => { let permissions = state.borrow_mut::(); @@ -446,7 +472,7 @@ impl Resource for FetchCancelHandle { } } -struct FetchRequestBodyResource { +pub struct FetchRequestBodyResource { body: AsyncRefCell>>>, cancel: CancelHandle, } diff --git a/runtime/build.rs b/runtime/build.rs index d32f72c9ff70a7..b0af848ba65c52 100644 --- a/runtime/build.rs +++ b/runtime/build.rs @@ -121,14 +121,14 @@ mod not_docs { deno_url::init(), deno_tls::init(), deno_web::init(deno_web::BlobStore::default(), Default::default()), - deno_fetch::init::( + deno_fetch::init::( "".to_owned(), None, None, None, None, None, - false, // No enable_file_fetch + deno_fetch::DefaultFileFetchHandler, // No enable_file_fetch ), deno_websocket::init::("".to_owned(), None, None), deno_webstorage::init(None), diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs index 797fde1443f0f9..8d3fcbb35a89f1 100644 --- a/runtime/web_worker.rs +++ b/runtime/web_worker.rs @@ -317,14 +317,14 @@ impl WebWorker { deno_console::init(), deno_url::init(), deno_web::init(options.blob_store.clone(), Some(main_module.clone())), - deno_fetch::init::( + deno_fetch::init::( options.user_agent.clone(), options.root_cert_store.clone(), None, None, options.unsafely_ignore_certificate_errors.clone(), None, - true, // enable_file_fetch + deno_fetch::FsFetchHandler, ), deno_websocket::init::( options.user_agent.clone(), diff --git a/runtime/worker.rs b/runtime/worker.rs index 9181622a92a612..1588896c8d59ed 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -101,14 +101,14 @@ impl MainWorker { options.blob_store.clone(), options.bootstrap.location.clone(), ), - deno_fetch::init::( + deno_fetch::init::( options.user_agent.clone(), options.root_cert_store.clone(), None, None, options.unsafely_ignore_certificate_errors.clone(), None, - true, // enabled_file_fetch + deno_fetch::FsFetchHandler, ), deno_websocket::init::( options.user_agent.clone(), From 15c61e54f72d03d4be624b6862c9edd9d17d2d57 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Fri, 29 Oct 2021 01:59:22 +0200 Subject: [PATCH 4/9] Streaming file fetch --- ext/fetch/fs_fetch_handler.rs | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/ext/fetch/fs_fetch_handler.rs b/ext/fetch/fs_fetch_handler.rs index 89d5ce79f0a03d..cd29456db9aa0f 100644 --- a/ext/fetch/fs_fetch_handler.rs +++ b/ext/fetch/fs_fetch_handler.rs @@ -9,7 +9,11 @@ use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_core::url::Url; +use deno_core::CancelFuture; +use reqwest::StatusCode; +use std::io; use std::rc::Rc; +use tokio_util::io::ReaderStream; /// An implementation which tries to read file URLs from the file system via /// tokio::fs @@ -25,25 +29,25 @@ impl FetchHandler for FsFetchHandler { Option, Option>, ) { - let path = url.to_file_path().unwrap(); + let cancel_handle = CancelHandle::new_rc(); + let response_fut = async move { - let response = { - match tokio::fs::read(&path).await { - Ok(body) => match http::Response::builder() - .status(http::StatusCode::OK) - .body(reqwest::Body::from(body)) - { - Ok(response) => Ok(reqwest::Response::from(response)), - Err(err) => Err(err.into()), - }, - Err(err) => Err(err.into()), - } - }; - Ok(response) + let path = url + .to_file_path() + .map_err(|()| io::Error::from(io::ErrorKind::NotFound))?; + let file = tokio::fs::File::open(path).await?; + let stream = ReaderStream::new(file); + let body = reqwest::Body::wrap_stream(stream); + let response = http::Response::builder() + .status(StatusCode::OK) + .body(body)? + .into(); + Ok::<_, AnyError>(response) } + .or_cancel(&cancel_handle) .boxed_local(); - (response_fut, None, None) + (response_fut, None, Some(cancel_handle)) } fn validate_url(&mut self, url: &Url) -> Result<(), AnyError> { From 0ae7e11acdd7ad3a652dff758c218fd07cb50558 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Fri, 29 Oct 2021 03:07:38 +0200 Subject: [PATCH 5/9] Fold url validation into fetch_url() --- ext/fetch/fs_fetch_handler.rs | 29 ++++++++--------------------- ext/fetch/lib.rs | 7 ------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/ext/fetch/fs_fetch_handler.rs b/ext/fetch/fs_fetch_handler.rs index cd29456db9aa0f..8d53f828002f62 100644 --- a/ext/fetch/fs_fetch_handler.rs +++ b/ext/fetch/fs_fetch_handler.rs @@ -6,12 +6,11 @@ use crate::FetchHandler; use crate::FetchRequestBodyResource; use deno_core::error::type_error; -use deno_core::error::AnyError; use deno_core::futures::FutureExt; +use deno_core::futures::TryFutureExt; use deno_core::url::Url; use deno_core::CancelFuture; use reqwest::StatusCode; -use std::io; use std::rc::Rc; use tokio_util::io::ReaderStream; @@ -30,36 +29,24 @@ impl FetchHandler for FsFetchHandler { Option>, ) { let cancel_handle = CancelHandle::new_rc(); + let url_ = url.clone(); let response_fut = async move { - let path = url - .to_file_path() - .map_err(|()| io::Error::from(io::ErrorKind::NotFound))?; - let file = tokio::fs::File::open(path).await?; + let path = url_.to_file_path()?; + let file = tokio::fs::File::open(path).map_err(|_| ()).await?; let stream = ReaderStream::new(file); let body = reqwest::Body::wrap_stream(stream); let response = http::Response::builder() .status(StatusCode::OK) - .body(body)? + .body(body) + .map_err(|_| ())? .into(); - Ok::<_, AnyError>(response) + Ok::<_, ()>(response) } + .map_err(move |_| type_error(format!("Unable to fetch \"{}\".", url))) .or_cancel(&cancel_handle) .boxed_local(); (response_fut, None, Some(cancel_handle)) } - - fn validate_url(&mut self, url: &Url) -> Result<(), AnyError> { - // Error messages are kept intentionally generic in order to discourage - // probing, and attempting to discern something from the environment. - let path = url - .to_file_path() - .map_err(|_| type_error(format!("Unable to fetch \"{}\".", url)))?; - if !path.is_file() { - Err(type_error(format!("Unable to fetch \"{}\".", url))) - } else { - Ok(()) - } - } } diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index d57a97e147cb2a..f6a9e9cf73099f 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -147,11 +147,6 @@ pub trait FetchHandler: Clone { }; (Box::pin(fut), None, None) } - - // Determine if a given URL is valid, returning an early error to the client - fn validate_url(&mut self, url: &Url) -> Result<(), AnyError> { - Err(type_error(format!("Unable to fetch \"{}\".", url))) - } } /// A default implementation which leverages the trait's default method @@ -227,8 +222,6 @@ where } let file_fetch_handler = state.borrow_mut::(); - file_fetch_handler.validate_url(&url)?; - let (request, maybe_request_body, maybe_cancel_handle) = file_fetch_handler.fetch_url(url); let request_rid = state.resource_table.add(FetchRequestResource(request)); From b66baa742a0fb66ad5504011b719b64e059bf8de Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Fri, 29 Oct 2021 20:40:52 +1100 Subject: [PATCH 6/9] Update ext/fetch/fs_fetch_handler.rs Co-authored-by: Luca Casonato --- ext/fetch/fs_fetch_handler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/fetch/fs_fetch_handler.rs b/ext/fetch/fs_fetch_handler.rs index 8d53f828002f62..bcf7772be91873 100644 --- a/ext/fetch/fs_fetch_handler.rs +++ b/ext/fetch/fs_fetch_handler.rs @@ -15,7 +15,7 @@ use std::rc::Rc; use tokio_util::io::ReaderStream; /// An implementation which tries to read file URLs from the file system via -/// tokio::fs +/// tokio::fs. #[derive(Clone)] pub struct FsFetchHandler; From 6864c321cbe32626145c2564abb3e3133395f753 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Sat, 30 Oct 2021 07:36:30 +1100 Subject: [PATCH 7/9] address more feedback --- ext/fetch/fs_fetch_handler.rs | 6 ++++-- ext/fetch/lib.rs | 36 ++++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/ext/fetch/fs_fetch_handler.rs b/ext/fetch/fs_fetch_handler.rs index bcf7772be91873..d1e6f21525bc2d 100644 --- a/ext/fetch/fs_fetch_handler.rs +++ b/ext/fetch/fs_fetch_handler.rs @@ -20,7 +20,7 @@ use tokio_util::io::ReaderStream; pub struct FsFetchHandler; impl FetchHandler for FsFetchHandler { - fn fetch_url( + fn fetch_file( &mut self, url: Url, ) -> ( @@ -43,7 +43,9 @@ impl FetchHandler for FsFetchHandler { .into(); Ok::<_, ()>(response) } - .map_err(move |_| type_error(format!("Unable to fetch \"{}\".", url))) + .map_err(move |_| { + type_error("NetworkError when attempting to fetch resource.") + }) .or_cancel(&cancel_handle) .boxed_local(); diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index f6a9e9cf73099f..e60fa1bbcb884f 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -134,7 +134,22 @@ pub trait FetchHandler: Clone { // Return the result of the fetch request consisting of a tuple of the // cancelable response result, the optional fetch body resource and the // optional cancel handle. - fn fetch_url( + fn fetch_file( + &mut self, + url: Url, + ) -> ( + CancelableResponseFuture, + Option, + Option>, + ); +} + +/// A default implementation which will error for every request. +#[derive(Clone)] +pub struct DefaultFileFetchHandler; + +impl FetchHandler for DefaultFileFetchHandler { + fn fetch_file( &mut self, url: Url, ) -> ( @@ -143,19 +158,14 @@ pub trait FetchHandler: Clone { Option>, ) { let fut = async move { - Ok(Err(type_error(format!("Unable to fetch \"{}\".", url)))) + Ok(Err(type_error( + "NetworkError when attempting to fetch resource.", + ))) }; (Box::pin(fut), None, None) } } -/// A default implementation which leverages the trait's default method -/// implementations. -#[derive(Clone)] -pub struct DefaultFileFetchHandler; - -impl FetchHandler for DefaultFileFetchHandler {} - pub trait FetchPermissions { fn check_net_url(&mut self, _url: &Url) -> Result<(), AnyError>; fn check_read(&mut self, _p: &Path) -> Result<(), AnyError>; @@ -208,9 +218,9 @@ where let scheme = url.scheme(); let (request_rid, request_body_rid, cancel_handle_rid) = match scheme { "file" => { - let path = url - .to_file_path() - .map_err(|_| type_error(format!("Invalid file URL: {}", url)))?; + let path = url.to_file_path().map_err(|_| { + type_error("NetworkError when attempting to fetch resource.") + })?; let permissions = state.borrow_mut::(); permissions.check_read(&path)?; @@ -223,7 +233,7 @@ where let file_fetch_handler = state.borrow_mut::(); let (request, maybe_request_body, maybe_cancel_handle) = - file_fetch_handler.fetch_url(url); + file_fetch_handler.fetch_file(url); let request_rid = state.resource_table.add(FetchRequestResource(request)); let maybe_request_body_rid = maybe_request_body.map(|r| state.resource_table.add(r)); From e67248970f80e89f470ca8489a9c1a15d30064db Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Sat, 30 Oct 2021 08:12:49 +1100 Subject: [PATCH 8/9] fix --- ext/fetch/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index e60fa1bbcb884f..b4bffb6de4e8fe 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -151,7 +151,7 @@ pub struct DefaultFileFetchHandler; impl FetchHandler for DefaultFileFetchHandler { fn fetch_file( &mut self, - url: Url, + _url: Url, ) -> ( CancelableResponseFuture, Option, From 43207a153c257da73a360f512250fe2ce055160e Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Sun, 31 Oct 2021 08:14:58 +1100 Subject: [PATCH 9/9] clippy --- ext/fetch/fs_fetch_handler.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ext/fetch/fs_fetch_handler.rs b/ext/fetch/fs_fetch_handler.rs index d1e6f21525bc2d..82cbc4ecb39305 100644 --- a/ext/fetch/fs_fetch_handler.rs +++ b/ext/fetch/fs_fetch_handler.rs @@ -29,10 +29,8 @@ impl FetchHandler for FsFetchHandler { Option>, ) { let cancel_handle = CancelHandle::new_rc(); - let url_ = url.clone(); - let response_fut = async move { - let path = url_.to_file_path()?; + let path = url.to_file_path()?; let file = tokio::fs::File::open(path).map_err(|_| ()).await?; let stream = ReaderStream::new(file); let body = reqwest::Body::wrap_stream(stream);