refactor(cache): implement registry isolation in cache directory #2406
Conversation
…add tests for new functionality
Summary of ChangesHello @justDemo-hjw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the cache management system to introduce registry-level isolation for stored packages. By embedding the registry host into the cache directory path, it resolves potential conflicts and integrity issues that arise when using multiple package registries. This enhancement improves the reliability and predictability of the caching mechanism, ensuring that packages from different sources are kept distinct. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces registry isolation in the cache directory, which is a valuable enhancement for users working with multiple package registries. The implementation is well-structured, and the changes are logical. I've identified a potential cross-platform compatibility issue in how registry URLs are converted to directory names, particularly when a port is present. Additionally, I've suggested an improvement to a test case to make it more robust in verifying the new functionality. Overall, these are great changes that improve the tool's flexibility.
| .strip_prefix("https://") | ||
| .or_else(|| registry_url.strip_prefix("http://")) | ||
| .unwrap_or(registry_url); | ||
| url.trim_end_matches('/').to_string() |
There was a problem hiding this comment.
The current implementation of registry_to_dir_name doesn't sanitize characters that are invalid in directory names on certain operating systems. For example, if a registry URL contains a port (e.g., http://localhost:4873), the resulting directory name localhost:4873 would be invalid on Windows due to the colon :. To ensure cross-platform compatibility, it's best to replace such characters.
You could also strengthen test_registry_to_dir_name by adding a test case for a URL with a port, assuming the fix is applied:
assert_eq!(
registry_to_dir_name("http://localhost:4873"),
"localhost_4873"
);| url.trim_end_matches('/').to_string() | |
| url.trim_end_matches('/').replace(':', "_").to_string() |
| let cache_dir = get_cache_dir(); | ||
| let cache_dir_str = cache_dir.to_string_lossy(); | ||
|
|
||
| // Should contain registry directory name | ||
| // The exact structure depends on current registry setting | ||
| assert!(cache_dir_str.contains("nm") || cache_dir_str.contains("cache")); |
There was a problem hiding this comment.
This test's assertion is a bit too lenient. assert!(cache_dir_str.contains("nm") || cache_dir_str.contains("cache")) would likely pass even with the old implementation, as the path already contained .cache/nm. A more robust test would be to verify that the generated cache directory path actually ends with the directory-sanitized registry name. This would more accurately confirm that registry isolation is working as intended.
| let cache_dir = get_cache_dir(); | |
| let cache_dir_str = cache_dir.to_string_lossy(); | |
| // Should contain registry directory name | |
| // The exact structure depends on current registry setting | |
| assert!(cache_dir_str.contains("nm") || cache_dir_str.contains("cache")); | |
| let cache_dir = get_cache_dir(); | |
| let registry = super::config::get_registry(); | |
| let registry_dir_name = registry_to_dir_name(®istry); | |
| assert!(cache_dir.ends_with(registry_dir_name)); |
xusd320
force-pushed
the
next
branch
2 times, most recently
from
f77808c to
567e5ae
Compare
1 participant
Fixes #2281
Summary
Currently, all packages are stored in
~/.cache/nm/without registry dimension isolation. This causes package confusion and integrity risks when switching between different registries (e.g.,registry.npmjs.organdregistry.npmmirror.com).This PR adds registry dimension isolation to the cache directory structure by including the registry host in the cache path. The cache structure changes from
~/.cache/nm/package-name/version/to~/.cache/nm/registry-host/package-name/version/.Changes:
registry_to_dir_name()function to convert registry URL to directory nameget_cache_dir()to return path with registry isolationcleancommand to useget_cache_dir()instead of hardcoded pathAll existing code using
get_cache_dir()automatically adapts to the new structure without modification, ensuring full backward compatibility.Test Plan
Unit tests: Added tests for
registry_to_dir_name()with various registry URL formats and verifiedget_cache_dir()includes registry dimension.Manual testing:
registry.npmmirror.com), verify cache path is~/.cache/nm/registry.npmmirror.com/package-name/version/registry.npmjs.organd install packages, verify cache path is~/.cache/nm/registry.npmjs.org/package-name/version/utoo clean, verify only current registry's cache is cleaned