summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Sherret <dsherret@users.noreply.github.com>2024-02-10 10:02:31 -0500
committerGitHub <noreply@github.com>2024-02-10 10:02:31 -0500
commitd2477f780630a812bfd65e3987b70c0d309385bb (patch)
treeaa3dcdff8929c724beeb6c5ade8bc0207fa8a516
parent34c8d1714026501a0d4907b7311006fadb25fcdf (diff)
fix: cache bust jsr deps on constraint failure (#22372)
Removes the `FileFetcher`'s internal cache because I don't believe it's necessary (we already cache this kind of stuff in places like deno_graph or config files in different places). Removing it fixes this bug because this functionality was already implemented in deno_graph and lowers memory usage of the CLI a little bit.
-rw-r--r--Cargo.lock1
-rw-r--r--Cargo.toml1
-rw-r--r--cli/Cargo.toml2
-rw-r--r--cli/file_fetcher.rs60
-rw-r--r--cli/tests/Cargo.toml1
-rw-r--r--cli/tests/integration/jsr_tests.rs76
-rw-r--r--cli/tests/testdata/jsr/version_not_found/main.out1
-rw-r--r--cli/tools/run/mod.rs4
-rw-r--r--cli/tools/test/mod.rs2
9 files changed, 98 insertions, 50 deletions
diff --git a/Cargo.lock b/Cargo.lock
index 979fffc16..93cac1a19 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -614,6 +614,7 @@ dependencies = [
"bytes",
"deno_ast",
"deno_bench_util",
+ "deno_cache_dir",
"deno_core",
"deno_fetch",
"deno_lockfile",
diff --git a/Cargo.toml b/Cargo.toml
index bf075418f..a43e1c178 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -97,6 +97,7 @@ chrono = { version = "0.4", default-features = false, features = ["std", "serde"
console_static_text = "=0.8.1"
data-encoding = "2.3.3"
data-url = "=0.3.0"
+deno_cache_dir = "=0.6.1"
dlopen2 = "0.6.1"
ecb = "=0.1.2"
elliptic-curve = { version = "0.13.4", features = ["alloc", "arithmetic", "ecdh", "std", "pem"] }
diff --git a/cli/Cargo.toml b/cli/Cargo.toml
index 7fe471dbe..25adbe57f 100644
--- a/cli/Cargo.toml
+++ b/cli/Cargo.toml
@@ -60,7 +60,7 @@ winres.workspace = true
[dependencies]
deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "proposal", "react", "sourcemap", "transforms", "typescript", "view", "visit"] }
-deno_cache_dir = "=0.6.1"
+deno_cache_dir = { workspace = true }
deno_config = "=0.9.2"
deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
deno_doc = { version = "=0.103.0", features = ["html"] }
diff --git a/cli/file_fetcher.rs b/cli/file_fetcher.rs
index 5a7ca2b84..8f4d3feab 100644
--- a/cli/file_fetcher.rs
+++ b/cli/file_fetcher.rs
@@ -100,20 +100,16 @@ impl File {
}
}
-/// Simple struct implementing in-process caching to prevent multiple
-/// fs reads/net fetches for same file.
#[derive(Debug, Clone, Default)]
-struct FileCache(Arc<Mutex<HashMap<ModuleSpecifier, File>>>);
+struct MemoryFiles(Arc<Mutex<HashMap<ModuleSpecifier, File>>>);
-impl FileCache {
+impl MemoryFiles {
pub fn get(&self, specifier: &ModuleSpecifier) -> Option<File> {
- let cache = self.0.lock();
- cache.get(specifier).cloned()
+ self.0.lock().get(specifier).cloned()
}
pub fn insert(&self, specifier: ModuleSpecifier, file: File) -> Option<File> {
- let mut cache = self.0.lock();
- cache.insert(specifier, file)
+ self.0.lock().insert(specifier, file)
}
}
@@ -157,7 +153,7 @@ pub struct FetchOptions<'a> {
pub struct FileFetcher {
auth_tokens: AuthTokens,
allow_remote: bool,
- cache: FileCache,
+ memory_files: MemoryFiles,
cache_setting: CacheSetting,
http_cache: Arc<dyn HttpCache>,
http_client: Arc<HttpClient>,
@@ -178,7 +174,7 @@ impl FileFetcher {
Self {
auth_tokens: AuthTokens::new(env::var("DENO_AUTH_TOKENS").ok()),
allow_remote,
- cache: Default::default(),
+ memory_files: Default::default(),
cache_setting,
http_cache,
http_client,
@@ -498,7 +494,7 @@ impl FileFetcher {
debug!("FileFetcher::fetch() - specifier: {}", specifier);
let scheme = get_validated_scheme(specifier)?;
options.permissions.check_specifier(specifier)?;
- if let Some(file) = self.cache.get(specifier) {
+ if let Some(file) = self.memory_files.get(specifier) {
Ok(file)
} else if scheme == "file" {
// we do not in memory cache files, as this would prevent files on the
@@ -514,7 +510,7 @@ impl FileFetcher {
format!("A remote specifier was requested: \"{specifier}\", but --no-remote is specified."),
))
} else {
- let result = self
+ self
.fetch_remote(
specifier,
options.permissions,
@@ -522,11 +518,7 @@ impl FileFetcher {
options.maybe_accept.map(String::from),
options.maybe_cache_setting.unwrap_or(&self.cache_setting),
)
- .await;
- if let Ok(file) = &result {
- self.cache.insert(specifier.clone(), file.clone());
- }
- result
+ .await
}
}
@@ -534,7 +526,7 @@ impl FileFetcher {
/// been cached in memory it will be returned, otherwise for local files will
/// be read from disk.
pub fn get_source(&self, specifier: &ModuleSpecifier) -> Option<File> {
- let maybe_file = self.cache.get(specifier);
+ let maybe_file = self.memory_files.get(specifier);
if maybe_file.is_none() {
let is_local = specifier.scheme() == "file";
if is_local {
@@ -548,9 +540,9 @@ impl FileFetcher {
}
}
- /// Insert a temporary module into the in memory cache for the file fetcher.
- pub fn insert_cached(&self, file: File) -> Option<File> {
- self.cache.insert(file.specifier.clone(), file)
+ /// Insert a temporary module for the file fetcher.
+ pub fn insert_memory_files(&self, file: File) -> Option<File> {
+ self.memory_files.insert(file.specifier.clone(), file)
}
}
@@ -826,7 +818,7 @@ mod tests {
"application/javascript".to_string(),
)])),
};
- file_fetcher.insert_cached(file.clone());
+ file_fetcher.insert_memory_files(file.clone());
let result = file_fetcher
.fetch(&specifier, PermissionsContainer::allow_all())
@@ -837,30 +829,6 @@ mod tests {
}
#[tokio::test]
- async fn test_get_source() {
- let _http_server_guard = test_util::http_server();
- let (file_fetcher, _) = setup(CacheSetting::Use, None);
- let specifier =
- resolve_url("http://localhost:4548/subdir/redirects/redirect1.js")
- .unwrap();
-
- let result = file_fetcher
- .fetch(&specifier, PermissionsContainer::allow_all())
- .await;
- assert!(result.is_ok());
-
- let maybe_file = file_fetcher.get_source(&specifier);
- assert!(maybe_file.is_some());
- let file = maybe_file.unwrap().into_text_decoded().unwrap();
- assert_eq!(file.source.as_ref(), "export const redirect = 1;\n");
- assert_eq!(
- file.specifier,
- resolve_url("http://localhost:4545/subdir/redirects/redirect1.js")
- .unwrap()
- );
- }
-
- #[tokio::test]
async fn test_fetch_data_url() {
let (file_fetcher, _) = setup(CacheSetting::Use, None);
let specifier = resolve_url("data:application/typescript;base64,ZXhwb3J0IGNvbnN0IGEgPSAiYSI7CgpleHBvcnQgZW51bSBBIHsKICBBLAogIEIsCiAgQywKfQo=").unwrap();
diff --git a/cli/tests/Cargo.toml b/cli/tests/Cargo.toml
index 6ad5984e2..578aaf47b 100644
--- a/cli/tests/Cargo.toml
+++ b/cli/tests/Cargo.toml
@@ -24,6 +24,7 @@ required-features = ["run"]
bytes.workspace = true
deno_ast.workspace = true
deno_bench_util.workspace = true
+deno_cache_dir = { workspace = true }
deno_core = { workspace = true, features = ["include_js_files_for_snapshotting", "unsafe_use_unprotected_platform"] }
deno_fetch.workspace = true
deno_lockfile.workspace = true
diff --git a/cli/tests/integration/jsr_tests.rs b/cli/tests/integration/jsr_tests.rs
index f7bdb6032..2de4f0056 100644
--- a/cli/tests/integration/jsr_tests.rs
+++ b/cli/tests/integration/jsr_tests.rs
@@ -1,7 +1,9 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
+use deno_core::serde_json::Value;
use deno_lockfile::Lockfile;
use test_util as util;
+use url::Url;
use util::env_vars_for_jsr_tests;
use util::TestContextBuilder;
@@ -105,3 +107,77 @@ console.log(version);"#,
.run()
.assert_matches_text("0.1.0\n");
}
+
+#[test]
+fn reload_info_not_found_cache_but_exists_remote() {
+ fn remove_version(registry_json: &mut Value, version: &str) {
+ registry_json
+ .as_object_mut()
+ .unwrap()
+ .get_mut("versions")
+ .unwrap()
+ .as_object_mut()
+ .unwrap()
+ .remove(version);
+ }
+
+ fn remove_version_for_package(
+ deno_dir: &util::TempDir,
+ package: &str,
+ version: &str,
+ ) {
+ let specifier =
+ Url::parse(&format!("http://127.0.0.1:4250/{}/meta.json", package))
+ .unwrap();
+ let registry_json_path = deno_dir
+ .path()
+ .join("deps")
+ .join(deno_cache_dir::url_to_filename(&specifier).unwrap());
+ let mut registry_json = registry_json_path.read_json_value();
+ remove_version(&mut registry_json, version);
+ registry_json_path.write_json(&registry_json);
+ }
+
+ // This tests that when a local machine doesn't have a version
+ // specified in a dependency that exists in the npm registry
+ let test_context = TestContextBuilder::for_jsr().use_temp_cwd().build();
+ let deno_dir = test_context.deno_dir();
+ let temp_dir = test_context.temp_dir();
+ temp_dir.write(
+ "main.ts",
+ "import { add } from 'jsr:@denotest/add@1'; console.log(add(1, 2));",
+ );
+
+ // cache successfully to the deno_dir
+ let output = test_context.new_command().args("cache main.ts").run();
+ output.assert_matches_text(concat!(
+ "Download http://127.0.0.1:4250/@denotest/add/meta.json\n",
+ "Download http://127.0.0.1:4250/@denotest/add/1.0.0_meta.json\n",
+ "Download http://127.0.0.1:4250/@denotest/add/1.0.0/mod.ts\n",
+ ));
+
+ // modify the package information in the cache to remove the latest version
+ remove_version_for_package(deno_dir, "@denotest/add", "1.0.0");
+
+ // should error when `--cache-only` is used now because the version is not in the cache
+ let output = test_context
+ .new_command()
+ .args("run --cached-only main.ts")
+ .run();
+ output.assert_exit_code(1);
+ output.assert_matches_text("error: Failed to resolve version constraint. Try running again without --cached-only
+ at file:///[WILDCARD]main.ts:1:21
+");
+
+ // now try running without it, it should download the package now
+ test_context
+ .new_command()
+ .args("run main.ts")
+ .run()
+ .assert_matches_text(concat!(
+ "Download http://127.0.0.1:4250/@denotest/add/meta.json\n",
+ "Download http://127.0.0.1:4250/@denotest/add/1.0.0_meta.json\n",
+ "3\n",
+ ))
+ .assert_exit_code(0);
+}
diff --git a/cli/tests/testdata/jsr/version_not_found/main.out b/cli/tests/testdata/jsr/version_not_found/main.out
index 5ebe13c73..6a32b5d81 100644
--- a/cli/tests/testdata/jsr/version_not_found/main.out
+++ b/cli/tests/testdata/jsr/version_not_found/main.out
@@ -1,4 +1,5 @@
Download http://127.0.0.1:4250/@denotest/deps/meta.json
+Download http://127.0.0.1:4250/@denotest/deps/meta.json
error: Could not find constraint in the list of versions: @denotest/deps@0.1.4
Specifier: jsr:@denotest/deps@0.1.4/mod.ts
at file:///[WILDCARD]/version_not_found/main.ts:1:19
diff --git a/cli/tools/run/mod.rs b/cli/tools/run/mod.rs
index ffb26f2eb..0de852fc2 100644
--- a/cli/tools/run/mod.rs
+++ b/cli/tools/run/mod.rs
@@ -91,7 +91,7 @@ pub async fn run_from_stdin(flags: Flags) -> Result<i32, AnyError> {
std::io::stdin().read_to_end(&mut source)?;
// Save a fake file into file fetcher cache
// to allow module access by TS compiler
- file_fetcher.insert_cached(File {
+ file_fetcher.insert_memory_files(File {
specifier: main_module.clone(),
maybe_headers: None,
source: source.into(),
@@ -174,7 +174,7 @@ pub async fn eval_command(
// Save a fake file into file fetcher cache
// to allow module access by TS compiler.
- file_fetcher.insert_cached(File {
+ file_fetcher.insert_memory_files(File {
specifier: main_module.clone(),
maybe_headers: None,
source: source_code.into_bytes().into(),
diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs
index c138abec2..2cf663b5f 100644
--- a/cli/tools/test/mod.rs
+++ b/cli/tools/test/mod.rs
@@ -873,7 +873,7 @@ pub async fn check_specifiers(
.collect();
for file in inline_files {
- file_fetcher.insert_cached(file);
+ file_fetcher.insert_memory_files(file);
}
module_load_preparer