Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: ensure metadata loading errors are correctly logged #2149

Merged
merged 1 commit into from
Jul 7, 2024
Merged

fix: ensure metadata loading errors are correctly logged #2149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
trace!(metadata=?env_metadata, "Loaded environment.");

let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws)?;

Check warning on line 67 in bin/sozo/src/commands/auth.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/auth.rs#L67 

Added line #L67 was not covered by tests

match self.command {
AuthCommand::Grant { kind, world, starknet, account, transaction } => {
Expand Down
28 changes: 21 additions & 7 deletions bin/sozo/src/commands/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,23 @@
pub fn run(self, config: &Config) -> Result<()> {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

if let Ok(current_package) = ws.current_package() {
if current_package.target(&TargetKind::new("dojo")).is_none() {
return Err(anyhow::anyhow!(
"No Dojo target found in the {} package. Add [[target.dojo]] to the {} \
manifest to enable Dojo features and compile with sozo.",
current_package.id.to_string(),
current_package.manifest_path()
));

Check warning on line 55 in bin/sozo/src/commands/build.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/build.rs#L50-L55 

Added lines #L50 - L55 were not covered by tests
}
}

Check warning on line 57 in bin/sozo/src/commands/build.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/build.rs#L57 

Added line #L57 was not covered by tests

// Namespaces are required to compute contracts/models data. Hence, we can't continue
// if no metadata are found.
// Once sozo will support package option, users will be able to do `-p` to select
// the package directly from the workspace instead of using `--manifest-path`.
let dojo_metadata = dojo_metadata_from_workspace(&ws)?;

let profile_name =
ws.current_profile().expect("Scarb profile is expected at this point.").to_string();

Expand Down Expand Up @@ -126,13 +143,10 @@
};
trace!(pluginManager=?bindgen, "Generating bindings.");

// Only generate bindgen if a current package is defined with dojo metadata.
if let Ok(dojo_metadata) = dojo_metadata_from_workspace(&ws) {
tokio::runtime::Runtime::new()
.unwrap()
.block_on(bindgen.generate(dojo_metadata.skip_migration))
.expect("Error generating bindings");
};
tokio::runtime::Runtime::new()
.unwrap()
.block_on(bindgen.generate(dojo_metadata.skip_migration))
.expect("Error generating bindings");

Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
self.tag_or_address
} else {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws)?;

Check warning on line 50 in bin/sozo/src/commands/call.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/call.rs#L50 

Added line #L50 was not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Consider adding error context for better debugging.

The addition of the ? operator ensures that errors are propagated correctly. However, adding context to the error can help with debugging.

let default_namespace = get_default_namespace_from_ws(&ws).context("Failed to get default namespace from workspace")?;

ensure_namespace(&self.tag_or_address, &default_namespace)
};

Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
self.tag_or_address
} else {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws)?;

Check warning on line 59 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L59 

Added line #L59 was not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Consider adding error context for better debugging.

The addition of the ? operator ensures that errors are propagated correctly. However, adding context to the error can help with debugging.

let default_namespace = get_default_namespace_from_ws(&ws).context("Failed to get default namespace from workspace")?;

ensure_namespace(&self.tag_or_address, &default_namespace)
};

Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
trace!(args = ?self);
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
let env_metadata = utils::load_metadata_from_config(config)?;
let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws)?;

Check warning on line 116 in bin/sozo/src/commands/model.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/model.rs#L116 

Added line #L116 was not covered by tests

config.tokio_handle().block_on(async {
match self.command {
Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/tests/register_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async fn reregister_models() {
let target_path =
ws.target_dir().path_existent().unwrap().join(ws.config().profile().to_string());

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();

let migration = prepare_migration(
source_project_dir.clone(),
Expand Down
2 changes: 1 addition & 1 deletion crates/dojo-lang/src/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl Compiler for DojoCompiler {
let props: Props = unit.main_component().target_props()?;
let target_dir = unit.target_dir(ws);

let default_namespace = get_default_namespace_from_ws(ws);
let default_namespace = get_default_namespace_from_ws(ws)?;

let compiler_config = build_compiler_config(&unit, ws);

Expand Down
25 changes: 15 additions & 10 deletions crates/dojo-world/src/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,9 @@
/// # Returns
///
/// A [`String`] object containing the namespace.
pub fn get_default_namespace_from_ws(ws: &Workspace<'_>) -> String {
let metadata = dojo_metadata_from_workspace(ws)
.expect("Namespace key is already checked by the parsing of the Scarb.toml file.");
metadata.world.namespace
pub fn get_default_namespace_from_ws(ws: &Workspace<'_>) -> Result<String> {
let metadata = dojo_metadata_from_workspace(ws)?;
Ok(metadata.world.namespace)
}

/// Build world metadata with data read from the project configuration.
Expand Down Expand Up @@ -99,13 +98,18 @@
let source_dir = source_dir.join(profile.as_str());

let project_metadata = if let Ok(current_package) = ws.current_package() {
current_package.manifest.metadata.dojo()?
current_package
.manifest
.metadata
.dojo()
.with_context(|| format!("Error parsing manifest file `{}`", ws.manifest_path()))?
} else {
// On workspaces, dojo metadata are not accessible because if no current package is defined
// (being the only package or using --package).
return Err(anyhow!(
"No current package with dojo metadata found, this subcommand is not yet support for \
workspaces."
"No current package with dojo metadata found, virtual manifest in workspace are not \
supported. Until package compilation is supported, you will have to provide the path \
to the Scarb.toml file using the --manifest-path option."

Check warning on line 112 in crates/dojo-world/src/metadata.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo-world/src/metadata.rs#L110-L112 

Added lines #L110 - L112 were not covered by tests
));
};

Expand Down Expand Up @@ -438,13 +442,14 @@
.tool_metadata
.as_ref()
.and_then(|e| e.get("dojo"))
// TODO: see if we can make error more descriptive
.ok_or_else(|| anyhow!("Some of the fields in [tool.dojo] are required."))?
.with_context(|| "No [tool.dojo] section found in the manifest.".to_string())?
.clone();

// The details of which field has failed to be loaded are logged inside the `try_into`
// error.
let project_metadata: ProjectMetadata = metadata
.try_into()
.with_context(|| "Project metadata (i.e. [tool.dojo]) is not properly configured.")?;
.with_context(|| "Project metadata [tool.dojo] is not properly configured.")?;

Ok(project_metadata)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/sozo/ops/src/migration/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ where
let target_dir = ws.target_dir().path_existent().unwrap();
let target_dir = target_dir.join(ws.config().profile().as_str());

let default_namespace = get_default_namespace_from_ws(ws);
let default_namespace = get_default_namespace_from_ws(ws)?;

// Load local and remote World manifests.
let (local_manifest, remote_manifest) = utils::load_world_manifests(
Expand Down
6 changes: 3 additions & 3 deletions crates/sozo/ops/src/tests/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ async fn migration_with_correct_calldata_second_time_work_as_expected() {
// adding correct calldata
manifest.merge(overlay);
}
let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();

let mut world = WorldDiff::compute(manifest, Some(remote_manifest));
world.update_order(&default_namespace).expect("Failed to update order");
Expand Down Expand Up @@ -375,7 +375,7 @@ async fn migrate_with_auto_authorize() {
let world_address = migration.world_address().expect("must be present");
let world = WorldContract::new(world_address, account);

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();
let res =
auto_authorize(&ws, &world, &txn_config, &manifest, &output, &default_namespace).await;
assert!(res.is_ok());
Expand Down Expand Up @@ -408,7 +408,7 @@ async fn migration_with_mismatching_world_address_and_seed() {
let base_dir = config.manifest_path().parent().unwrap().to_path_buf();
let target_dir = base_dir.join("target").join("dev");

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();

let strategy = prepare_migration_with_world_and_seed(
base_dir,
Expand Down
2 changes: 1 addition & 1 deletion crates/sozo/ops/src/tests/setup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub fn setup_migration(config: &Config) -> Result<MigrationStrategy> {
let base_dir = manifest_path.parent().unwrap();
let target_dir = format!("{}/target/dev", base_dir);

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a meaningful error message.

Using .unwrap() in test setups is generally acceptable, but providing a meaningful error message can make debugging easier.

let default_namespace = get_default_namespace_from_ws(&ws).expect("Failed to get default namespace from workspace");


prepare_migration_with_world_and_seed(
base_dir.into(),
Expand Down
4 changes: 2 additions & 2 deletions crates/torii/core/src/sql_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ async fn test_load_from_remote() {
let base_dir = manifest_path.parent().unwrap();
let target_dir = format!("{}/target/dev", base_dir);

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();

let mut migration = prepare_migration(
base_dir.into(),
Expand Down Expand Up @@ -222,7 +222,7 @@ async fn test_load_from_remote_del() {
let base_dir = manifest_path.parent().unwrap();
let target_dir = format!("{}/target/dev", base_dir);

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();

let mut migration = prepare_migration(
base_dir.into(),
Expand Down
2 changes: 1 addition & 1 deletion crates/torii/graphql/src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ pub async fn spinup_types_test() -> Result<SqlitePool> {

let target_path = ws.target_dir().path_existent().unwrap().join(config.profile().to_string());

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();

let mut migration = prepare_migration(
source_project_dir,
Expand Down
2 changes: 1 addition & 1 deletion crates/torii/grpc/src/server/tests/entities_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ async fn test_entities_queries() {

let target_path = ws.target_dir().path_existent().unwrap().join(config.profile().to_string());

let default_namespace = get_default_namespace_from_ws(&ws);
let default_namespace = get_default_namespace_from_ws(&ws).unwrap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a meaningful error message.

Using .unwrap() in tests is generally acceptable, but providing a meaningful error message can make debugging easier.

let default_namespace = get_default_namespace_from_ws(&ws).expect("Failed to get default namespace from workspace");


let mut migration = prepare_migration(
config.manifest_path().parent().unwrap().into(),
Expand Down
Loading