-
Notifications
You must be signed in to change notification settings - Fork 1k
gnd: Support multiple subgraphs, grafting, subgraph composition in dev mode #6000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: krishna/graph-dev
Are you sure you want to change the base?
Changes from all commits
a699ce7
eca8191
17e02ee
9256d76
f32f798
1b6f97e
caaf26c
b5bbf93
14e01ac
6fda901
91eefaf
58311ff
f4ce848
a656738
1423ae3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
use std::collections::HashMap; | ||
use std::path::{Path, PathBuf}; | ||
use std::time::Duration; | ||
|
||
|
@@ -6,22 +7,35 @@ use async_trait::async_trait; | |
use slog::Logger; | ||
|
||
use crate::data::subgraph::Link; | ||
use crate::prelude::{Error, JsonValueStream, LinkResolver as LinkResolverTrait}; | ||
use crate::prelude::{DeploymentHash, Error, JsonValueStream, LinkResolver as LinkResolverTrait}; | ||
|
||
#[derive(Clone, Debug)] | ||
pub struct FileLinkResolver { | ||
base_dir: Option<PathBuf>, | ||
timeout: Duration, | ||
// This is a hashmap that maps the alias name to the path of the file that is aliased | ||
aliases: HashMap<String, PathBuf>, | ||
} | ||
|
||
impl Default for FileLinkResolver { | ||
fn default() -> Self { | ||
Self { | ||
base_dir: None, | ||
timeout: Duration::from_secs(30), | ||
aliases: HashMap::new(), | ||
} | ||
} | ||
} | ||
|
||
impl FileLinkResolver { | ||
/// Create a new FileLinkResolver | ||
/// | ||
/// All paths are treated as absolute paths. | ||
pub fn new() -> Self { | ||
pub fn new(base_dir: Option<PathBuf>, aliases: HashMap<String, PathBuf>) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be clearer if |
||
Self { | ||
base_dir: None, | ||
base_dir: base_dir, | ||
timeout: Duration::from_secs(30), | ||
aliases, | ||
} | ||
} | ||
|
||
|
@@ -33,12 +47,18 @@ impl FileLinkResolver { | |
Self { | ||
base_dir: Some(base_dir.as_ref().to_owned()), | ||
timeout: Duration::from_secs(30), | ||
aliases: HashMap::new(), | ||
} | ||
} | ||
|
||
fn resolve_path(&self, link: &str) -> PathBuf { | ||
let path = Path::new(link); | ||
|
||
// If the path is an alias, use the aliased path | ||
if let Some(aliased) = self.aliases.get(link) { | ||
return aliased.clone(); | ||
} | ||
|
||
// If the path is already absolute or if we don't have a base_dir, return it as is | ||
if path.is_absolute() || self.base_dir.is_none() { | ||
path.to_owned() | ||
|
@@ -47,6 +67,42 @@ impl FileLinkResolver { | |
self.base_dir.as_ref().unwrap().join(link) | ||
} | ||
} | ||
|
||
/// This method creates a new resolver that is scoped to a specific subgraph | ||
/// It will set the base directory to the parent directory of the manifest path | ||
/// This is required because paths mentioned in the subgraph manifest are relative paths | ||
/// and we need a new resolver with the right base directory for the specific subgraph | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't followed the flow of the code overall too closely, but is there any way where we could first find the manifest we are interested in and then create the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lutter I dont follow your suggestion here. Are you suggesting getting rid of the aliases and instead just relying on the path of the manifest for creating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I confused myself by looking at early commits. I think the setup with |
||
fn clone_for_deployment(&self, deployment: DeploymentHash) -> Result<Self, Error> { | ||
let mut resolver = self.clone(); | ||
|
||
let deployment_str = deployment.to_string(); | ||
|
||
// Create a path to the manifest based on the current resolver's | ||
// base directory or default to using the deployment string as path | ||
// If the deployment string is an alias, use the aliased path | ||
let manifest_path = if let Some(aliased) = self.aliases.get(&deployment_str) { | ||
aliased.clone() | ||
} else { | ||
match &resolver.base_dir { | ||
Some(dir) => dir.join(&deployment_str), | ||
None => PathBuf::from(deployment_str), | ||
} | ||
}; | ||
|
||
let canonical_manifest_path = manifest_path | ||
.canonicalize() | ||
.map_err(|e| Error::from(anyhow!("Failed to canonicalize manifest path: {}", e)))?; | ||
|
||
// The manifest path is the path of the subgraph manifest file in the build directory | ||
// We use the parent directory as the base directory for the new resolver | ||
let base_dir = canonical_manifest_path | ||
.parent() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit confused now, isn't this basically There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deployment_str can be something like When deployment_str is an absolute path its simply the directory in which the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't |
||
.ok_or_else(|| Error::from(anyhow!("Manifest path has no parent directory")))? | ||
.to_path_buf(); | ||
|
||
resolver.base_dir = Some(base_dir); | ||
Ok(resolver) | ||
} | ||
} | ||
|
||
pub fn remove_prefix(link: &str) -> &str { | ||
|
@@ -87,6 +143,13 @@ impl LinkResolverTrait for FileLinkResolver { | |
} | ||
} | ||
|
||
fn for_deployment( | ||
&self, | ||
deployment: DeploymentHash, | ||
) -> Result<Box<dyn LinkResolverTrait>, Error> { | ||
Ok(Box::new(self.clone_for_deployment(deployment)?)) | ||
} | ||
|
||
async fn get_block(&self, _logger: &Logger, _link: &Link) -> Result<Vec<u8>, Error> { | ||
Err(anyhow!("get_block is not implemented for FileLinkResolver").into()) | ||
} | ||
|
@@ -118,7 +181,7 @@ mod tests { | |
file.write_all(test_content).unwrap(); | ||
|
||
// Create a resolver without a base directory | ||
let resolver = FileLinkResolver::new(); | ||
let resolver = FileLinkResolver::default(); | ||
let logger = slog::Logger::root(slog::Discard, slog::o!()); | ||
|
||
// Test valid path resolution | ||
|
@@ -220,4 +283,65 @@ mod tests { | |
let _ = fs::remove_file(test_file_path); | ||
let _ = fs::remove_dir(temp_dir); | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_file_resolver_with_aliases() { | ||
// Create a temporary directory for test files | ||
let temp_dir = env::temp_dir().join("file_resolver_test_aliases"); | ||
let _ = fs::create_dir_all(&temp_dir); | ||
|
||
// Create two test files with different content | ||
let test_file1_path = temp_dir.join("file.txt"); | ||
let test_content1 = b"This is the file content"; | ||
let mut file1 = fs::File::create(&test_file1_path).unwrap(); | ||
file1.write_all(test_content1).unwrap(); | ||
|
||
let test_file2_path = temp_dir.join("another_file.txt"); | ||
let test_content2 = b"This is another file content"; | ||
let mut file2 = fs::File::create(&test_file2_path).unwrap(); | ||
file2.write_all(test_content2).unwrap(); | ||
|
||
// Create aliases mapping | ||
let mut aliases = HashMap::new(); | ||
aliases.insert("alias1".to_string(), test_file1_path.clone()); | ||
aliases.insert("alias2".to_string(), test_file2_path.clone()); | ||
aliases.insert("deployment-id".to_string(), test_file1_path.clone()); | ||
|
||
// Create resolver with aliases | ||
let resolver = FileLinkResolver::new(Some(temp_dir.clone()), aliases); | ||
let logger = slog::Logger::root(slog::Discard, slog::o!()); | ||
|
||
// Test resolving by aliases | ||
let link1 = Link { | ||
link: "alias1".to_string(), | ||
}; | ||
let result1 = resolver.cat(&logger, &link1).await.unwrap(); | ||
assert_eq!(result1, test_content1); | ||
|
||
let link2 = Link { | ||
link: "alias2".to_string(), | ||
}; | ||
let result2 = resolver.cat(&logger, &link2).await.unwrap(); | ||
assert_eq!(result2, test_content2); | ||
|
||
// Test that the alias works in for_deployment as well | ||
let deployment = DeploymentHash::new("deployment-id").unwrap(); | ||
let deployment_resolver = resolver.clone_for_deployment(deployment).unwrap(); | ||
|
||
let expected_dir = test_file1_path.parent().unwrap(); | ||
let deployment_base_dir = deployment_resolver.base_dir.clone().unwrap(); | ||
|
||
let canonical_expected_dir = expected_dir.canonicalize().unwrap(); | ||
let canonical_deployment_dir = deployment_base_dir.canonicalize().unwrap(); | ||
|
||
assert_eq!( | ||
canonical_deployment_dir, canonical_expected_dir, | ||
"Build directory paths don't match" | ||
); | ||
|
||
// Clean up | ||
let _ = fs::remove_file(test_file1_path); | ||
let _ = fs::remove_file(test_file2_path); | ||
let _ = fs::remove_dir(temp_dir); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of threading
ignore_graft_base
through, you could also just manipulate theraw
here and remove any mention of a graft