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.

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

feat(torii-core): specify namespaces to exclusively index #2687

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

Larkooo
Copy link
Collaborator

@Larkooo Larkooo commented Nov 14, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new namespaces field in event processing configurations for enhanced categorization and filtering.
  • Improvements

    • Simplified handling of the historical events field by removing optionality, ensuring it always contains a vector.
    • Updated configuration merging logic for clarity and improved default settings.
    • Enhanced error handling in event processing methods to gracefully ignore missing models without propagating errors.
  • Documentation

    • Added comments and documentation for new and updated fields to enhance user understanding.

Copy link

coderabbitai bot commented Nov 14, 2024

Walkthrough

Ohayo, sensei! This pull request introduces changes to the event processing configuration in the Torii application. Key modifications include the removal of optionality for the historical field in the EventsOptions struct and the addition of a new namespaces field in both EventProcessorConfig and IndexingOptions. These adjustments streamline the configuration handling, ensuring that the application directly utilizes vectors instead of optional vectors, which may impact how events are processed during execution.

Changes

File Path Change Summary
bin/torii/src/main.rs - Modified historical_events handling in EventProcessorConfig from unwrap_or_default() to into_iter().
- Added namespaces field to EventProcessorConfig.
crates/torii/cli/src/args.rs - Changed historical field in EventsOptions from Option<Vec<String>> to Vec<String>.
- Updated test assertions to reflect renaming of indexing fields.
crates/torii/cli/src/options.rs - Added namespaces field to IndexingOptions.
- Updated historical field in EventsOptions to be a non-optional Vec<String>.
crates/torii/core/src/processors/mod.rs - Added namespaces field to EventProcessorConfig as a HashSet<String>.
crates/torii/core/src/processors/register_event.rs - Updated process method to rename _config to config and added namespace filtering logic.
crates/torii/core/src/processors/register_model.rs - Updated process method to rename _config to config and added namespace filtering logic.
crates/torii/core/src/processors/store_del_record.rs - Modified error handling in process method to ignore "no rows" errors when retrieving models.
crates/torii/core/src/processors/store_set_record.rs - Modified error handling in process method to ignore "no rows" errors when retrieving models.
crates/torii/core/src/processors/store_update_member.rs - Modified error handling in process method to ignore "no rows" errors when retrieving models.
crates/torii/core/src/processors/store_update_record.rs - Modified error handling in process method to ignore "no rows" errors when retrieving models.

Possibly related PRs

  • feat(torii-core): bitflags for indexing #2450: This PR introduces new command-line arguments for indexing transactions and events, which is related to the changes in the main PR that also involve indexing flags in the EventProcessorConfig.

Suggested reviewers

  • glihm

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
crates/torii/cli/src/args.rs (1)

285-285: Consider adding namespace format validation, sensei!

The test demonstrates namespace patterns like "ns-E" and "ns-EH", but there's no validation to ensure consistent formatting. Consider adding validation to prevent potential issues.

+ // Add this validation in EventsOptions or where the historical field is processed
+ fn validate_namespace(ns: &str) -> bool {
+     // Adjust the regex pattern based on your naming convention
+     lazy_static! {
+         static ref NS_PATTERN: Regex = Regex::new(r"^[a-zA-Z]+-[a-zA-Z]+$").unwrap();
+     }
+     NS_PATTERN.is_match(ns)
+ }
crates/torii/cli/src/options.rs (1)

142-148: Ohayo! Consider enhancing the help text for namespaces

The help text could be more specific about the expected format of namespace strings to ensure users provide correct input.

     #[arg(
         long = "indexing.namespaces",
         value_delimiter = ',',
-        help = "The namespaces of the world that torii should index. If empty, all namespaces will be indexed."
+        help = "Comma-separated list of world namespaces that torii should index (e.g., 'namespace1,namespace2'). If empty, all namespaces will be indexed."
     )]
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4671d9c and d87c399.

📒 Files selected for processing (4)
  • bin/torii/src/main.rs (1 hunks)
  • crates/torii/cli/src/args.rs (2 hunks)
  • crates/torii/cli/src/options.rs (3 hunks)
  • crates/torii/core/src/processors/mod.rs (1 hunks)
🔇 Additional comments (5)
crates/torii/core/src/processors/mod.rs (2)

32-32: Ohayo! The new namespaces field looks good, sensei!

The addition of pub namespaces: HashSet<String> aligns well with the PR objective of specifying namespaces to exclusively index. The use of HashSet is appropriate for storing unique namespace identifiers.


Line range hint 52-61: Consider utilizing the config parameter in EventProcessor::process

Ohayo sensei! I notice that the _config parameter in the process method is prefixed with an underscore, indicating it's currently unused. Since we've added the new namespaces field, this parameter could be valuable for namespace-based filtering.

Let's check if any implementations are using this parameter:

Consider implementing namespace filtering logic using this parameter if appropriate for your use case.

bin/torii/src/main.rs (1)

147-148: Ohayo sensei! Please verify the safety of these changes and add documentation.

The changes to EventProcessorConfig initialization have two important aspects:

  1. The removal of unwrap_or_default() for historical_events suggests this is now a required field. Please ensure all callers are updated to handle this breaking change.

  2. The new namespaces field adds filtering capability but lacks documentation about its purpose and expected values.

Let's verify the impact of these changes:

Consider adding documentation to explain:

  • The purpose and behavior of namespace filtering
  • Expected format and values for namespaces
  • Impact of an empty namespace collection
✅ Verification successful

Ohayo sensei! The changes are safe but need documentation love!

Based on the codebase analysis:

  1. The historical field in EventsOptions is already a non-optional vector (CLI arg with value_delimiter), so removing unwrap_or_default() is safe.
  2. Both fields are properly documented in their source structs:
    • historical_events has clear documentation in options.rs
    • However, namespaces in EventProcessorConfig lacks documentation

Please add documentation for the namespaces field in crates/torii/core/src/processors/mod.rs to explain:

/// A set of namespace identifiers to exclusively process events from.
/// Empty set means all namespaces are processed.
pub namespaces: HashSet<String>,
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential breaking changes and documentation

# Check if there are any callers still using the optional pattern
rg -A 2 "historical\?: Option<" 

# Check if there's documentation for the new namespaces field
ast-grep --pattern 'struct EventProcessorConfig {
  $$$
  /// $DOC_COMMENT
  namespaces: $TYPE
  $$$
}'

Length of output: 139


Script:

#!/bin/bash
# Let's try a different approach to find the relevant structs and their usage

# Find EventsOptions struct definition
rg -A 10 "struct EventsOptions"

# Find EventProcessorConfig struct definition
rg -A 10 "struct EventProcessorConfig"

# Check for any documentation about namespaces in the codebase
rg -i "namespace.*filter" --type rust

# Look for historical events usage patterns
rg "historical_events" --type rust -A 2

Length of output: 2481

crates/torii/cli/src/args.rs (1)

239-239: Ohayo! Verify backward compatibility with existing configurations, sensei.

The change from Option<Vec<String>> to Vec<String> for historical events makes the field mandatory. While this aligns with the goal of explicit namespace specification, we should ensure existing configurations won't break.

✅ Verification successful

Ohayo! The change is safe and backward compatible, sensei!

The codebase analysis shows that:

  1. historical field in EventsOptions has a default empty vector implementation
  2. The field is properly handled throughout the codebase in both GRPC and core implementations
  3. Existing configurations use explicit array syntax for historical events, making them compatible with the new non-optional type
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing torii config files in the codebase
fd -e toml -e json . | while read -r config_file; do
  echo "Checking $config_file for historical events configuration:"
  rg -A 2 'historical.*=' "$config_file"
done

Length of output: 36595


Script:

#!/bin/bash
# Let's check the implementation and usage of historical events in the codebase
# First, let's find the struct/type definition
rg -A 5 "struct.*Events" crates/torii

# Then check for historical events usage in tests and implementation
rg -A 5 "historical.*=|historical:" crates/torii

# Finally, check for any migration code or breaking changes
rg -B 5 "historical" crates/torii/cli/src/args.rs

Length of output: 15371

crates/torii/cli/src/options.rs (1)

180-180: Ohayo sensei! Breaking change alert for historical field

The change from Option<Vec<String>> to Vec<String> improves code clarity by removing unnecessary optionality. However, this is a breaking change that may affect existing configurations.

Let's check for any existing configurations that might be affected:

Also applies to: 185-185

✅ Verification successful

The initial search revealed a configuration file using historical_events. I'd like to verify the code usage and test configurations to better understand the impact of this breaking change before providing the final response.


Ohayo sensei! Breaking change confirmed but safely handled

The change from Option<Vec<String>> to Vec<String> is indeed a breaking change, but the codebase appears to handle it gracefully:

  • The existing configuration in bin/torii/torii.toml uses historical_events=[...] format which is compatible with Vec<String>
  • The change is well-contained within the CLI configuration layer and doesn't propagate to the core functionality, as evidenced by the historical field usage across the codebase
  • Test cases in crates/torii/cli/src/args.rs already expect non-optional vectors
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for existing usage of historical field in configurations
# Look for TOML/YAML/JSON config files that might use the historical field
rg -g '*.{toml,yaml,yml,json}' -B2 -A2 'historical.*='

Length of output: 248


Script:

#!/bin/bash
# Let's check the historical field usage in the code to understand the impact
ast-grep --pattern 'historical'

# Also check for any test configurations
fd -e rs -x rg -l 'historical'

Length of output: 4321

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (6)
crates/torii/core/src/processors/store_del_record.rs (1)

Line range hint 1-85: Consider adding tracing for ignored models, sensei! 📝

While the implementation is solid, it might be helpful to add debug-level tracing when models are ignored due to namespace filtering. This would aid in debugging and monitoring.

Here's a suggested enhancement:

            Err(e) => {
                if e.to_string().contains("no rows") {
+                   tracing::debug!(
+                       target: LOG_TARGET,
+                       selector = %event.selector,
+                       "Ignoring non-existent model due to namespace filtering"
+                   );
                    return Ok(());
                }
                return Err(e);
            }
crates/torii/core/src/processors/store_set_record.rs (1)

55-57: Consider documenting the namespace filtering behavior, sensei! 📚

Since this change is part of the namespace-specific indexing feature, it might be valuable to:

  1. Document this behavior in the processor's documentation
  2. Consider adding metrics/logging to track skipped records for monitoring
  3. Consider adding a debug log when skipping records to help with troubleshooting
crates/torii/core/src/processors/store_update_record.rs (1)

63-63: Consider more robust error type matching, sensei!

The current approach of string matching on error messages (contains("no rows")) is somewhat fragile. Consider either:

  1. Using a proper error type from your database layer that can be matched explicitly
  2. Creating a custom error type for namespace filtering scenarios

Example approach:

#[derive(Error, Debug)]
pub enum ModelError {
    #[error("model not found in indexed namespaces")]
    NotInIndexedNamespace(String),
    #[error("database error: {0}")]
    DatabaseError(#[from] sqlx::Error),
}
crates/torii/core/src/processors/register_model.rs (1)

61-65: Namespace filtering implementation looks solid, sensei!

The logic effectively implements selective namespace indexing while maintaining good defaults (empty list = index all). Consider these enhancements:

  1. Add debug logging when skipping namespaces for better observability
  2. Consider adding metrics/counters for skipped events
 // If the namespace is not in the list of namespaces to index, silently ignore it.
 // If our config is empty, we index all namespaces.
 if !config.namespaces.is_empty() && !config.namespaces.contains(&namespace) {
+    debug!(
+        target: LOG_TARGET,
+        %namespace,
+        "Skipping model registration due to namespace filter"
+    );
     return Ok(());
 }
crates/torii/core/src/processors/register_event.rs (1)

61-65: Consider adding debug logging for filtered events, sensei!

The namespace filtering logic is well-implemented with clear comments and correct handling of empty config case. However, adding debug logging when events are filtered out would help with observability.

 if !config.namespaces.is_empty() && !config.namespaces.contains(&namespace) {
+    debug!(
+        target: LOG_TARGET,
+        %namespace,
+        "Skipping event from non-indexed namespace"
+    );
     return Ok(());
 }
crates/torii/core/src/processors/store_update_member.rs (1)

59-69: Consider documenting the error handling pattern, sensei!

Since this error handling approach is specific to namespace-aware indexing, it would be valuable to document this pattern in a central location (like README.md or CONTRIBUTING.md) to ensure consistency across all processors.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d87c399 and 115913d.

📒 Files selected for processing (7)
  • crates/torii/cli/src/options.rs (3 hunks)
  • crates/torii/core/src/processors/register_event.rs (2 hunks)
  • crates/torii/core/src/processors/register_model.rs (2 hunks)
  • crates/torii/core/src/processors/store_del_record.rs (2 hunks)
  • crates/torii/core/src/processors/store_set_record.rs (2 hunks)
  • crates/torii/core/src/processors/store_update_member.rs (1 hunks)
  • crates/torii/core/src/processors/store_update_record.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/torii/cli/src/options.rs
🔇 Additional comments (8)
crates/torii/core/src/processors/store_del_record.rs (2)

54-64: Ohayo! The error handling looks good, sensei! 🎋

The graceful handling of missing models aligns well with the namespace-specific indexing feature. The explicit comments and structured error handling make the intention clear.


56-63: Verify error string matching reliability, sensei! 🔍

The current implementation relies on string matching to detect "no rows" errors. While functional, this approach might be fragile if the error message changes.

Let's check if this error string is consistently used across the codebase:

✅ Verification successful

Let me search for the database error handling implementation to understand where this error originates.


Let me search for the actual database error type and its implementation to understand where this error message originates.


Let me search for the SQL error handling implementation to understand where this error message comes from.


Ohayo sensei! The error string matching is actually reliable! 🎯

The string matching for "no rows" is consistently used across the codebase and originates from sqlx's RowNotFound error. This is evident from:

  1. The error handling pattern is consistently used in multiple processors:

    • store_del_record.rs
    • store_update_record.rs
    • store_set_record.rs
    • store_update_member.rs
  2. The error comes from sqlx (SQL toolkit for Rust) as seen in the server implementation:

Error::Sql(sqlx::Error::RowNotFound)

This is a standard SQL error pattern and the string matching is reliable since it's tied to sqlx's core error type.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar error string matching patterns
rg -A 2 'contains.*no rows' 

# Search for the actual error string definition
rg '"no rows"'

Length of output: 1579


Script:

#!/bin/bash
# Search for database error type definitions and implementations
rg -A 5 'impl.*Error' 

# Search for database-related error handling
rg -A 5 'type.*Error'

# Search for error conversion implementations
rg -A 5 'impl.*From.*Error'

Length of output: 105433


Script:

#!/bin/bash
# Search for database error type definitions and implementations
rg -A 5 'pub.*struct.*Error'
rg -A 5 'pub.*enum.*Error'

# Search for database-related modules and implementations
rg -A 5 'mod.*db'

Length of output: 47216


Script:

#!/bin/bash
# Search for SQL-related error handling and implementations
rg -A 5 'sqlx::Error'
rg -A 5 'impl.*From.*sqlx::Error'
rg -A 5 'no rows'

Length of output: 3321

crates/torii/core/src/processors/store_set_record.rs (1)

55-65: Ohayo! The error handling improvement looks good, sensei! 🎋

The new error handling gracefully handles the case where a model doesn't exist, which is expected behavior when indexing specific namespaces. The comments clearly explain the rationale.

Let's verify this pattern is consistent across other processors:

✅ Verification successful

The previous search might have been too specific. Let's try a broader pattern to catch similar error handling approaches and also identify which processor files interact with models that might need consistent error handling.


Let's examine how each of these files handles model-related errors to ensure consistency in error handling across processors.


Let's try a more specific pattern to catch the model-related error handling, focusing on the actual database calls and their surrounding context.


Ohayo! The error handling pattern is well-aligned across processors, sensei! 🎋

The verification shows consistent error handling for missing models across the processors:

  • store_update_member.rs
  • store_del_record.rs
  • store_update_record.rs
  • event_message.rs

All processors that need to handle missing models implement the same pattern of silently ignoring "no rows" errors, with appropriate comments explaining the rationale. The remaining processors (register_event.rs and register_model.rs) don't need this handling as they don't query existing models.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if similar error handling exists in other processor files
# Expected: Similar error handling in other processor files

# Search for similar error handling patterns in other processor files
rg -U "match.*model.*await.*\{.*contains.*no rows.*Ok\(\(\)\).*\}" crates/torii/core/src/processors/

Length of output: 102


Script:

#!/bin/bash
# Let's try a more flexible search pattern to find similar error handling in processor files
rg -U "match\s+.*model.*\{.*no rows.*\}" crates/torii/core/src/processors/ -A 5

# Also search for files that might need similar error handling
fd ".*\.rs$" crates/torii/core/src/processors/ --exec grep -l "model(" {} \;

Length of output: 533


Script:

#!/bin/bash
# Let's check the error handling in each file that uses model()
for file in store_update_member.rs store_del_record.rs store_update_record.rs register_event.rs register_model.rs event_message.rs; do
    echo "=== $file ==="
    rg -U "let\s+model\s*=.*?\{.*?\}" "crates/torii/core/src/processors/$file" -A 5
done

Length of output: 1748


Script:

#!/bin/bash
# Let's try a different approach to find model error handling
for file in store_update_member.rs store_del_record.rs store_update_record.rs register_event.rs register_model.rs event_message.rs; do
    echo "=== $file ==="
    rg "db\.model.*await" "crates/torii/core/src/processors/$file" -B 2 -A 10
done

Length of output: 3395

crates/torii/core/src/processors/store_update_record.rs (1)

58-68: Ohayo! Verify error handling for namespace-specific indexing

The new error handling gracefully handles non-existent models, which aligns with the namespace-specific indexing feature. However, we might want to add debug-level logging here to maintain observability.

 let model = match db.model(event.selector).await {
     Ok(m) => m,
     Err(e) => {
         if e.to_string().contains("no rows") {
+            tracing::debug!(
+                target: LOG_TARGET,
+                selector = %event.selector,
+                "Ignoring model update for non-indexed namespace"
+            );
             return Ok(());
         }
         return Err(e);
     }
 };

Let's verify the behavior across other processors:

✅ Verification successful

Let me check if there's any logging in these files to ensure consistency.


Ohayo! The error handling pattern is consistent across processors, sensei!

The verification shows that:

  1. All processor files consistently handle the "no rows" case in the same way
  2. Each processor has its own LOG_TARGET constant and proper tracing setup
  3. Other processors already use logging for important events

Since this is a consistent pattern across the codebase and the error handling aligns with the namespace-specific indexing feature, the suggestion for adding debug logging is good but not critical. The current implementation maintains consistency with other processors.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if similar error handling exists in other processors
# Expected: Find similar patterns in other processor files

rg -A 5 "contains.*no rows" crates/torii/core/src/processors/

Length of output: 2036


Script:

#!/bin/bash
# Check for logging patterns in processor files
rg -B 2 -A 2 "tracing::(debug|info|warn|error)" crates/torii/core/src/processors/

Length of output: 3428


Script:

#!/bin/bash
# Check for LOG_TARGET constant
rg "LOG_TARGET" crates/torii/core/src/processors/

Length of output: 3572


Script:

#!/bin/bash
# Check the module structure to see if logging is defined at a higher level
ast-grep --pattern 'const LOG_TARGET: &str = $_'

Length of output: 299

crates/torii/core/src/processors/register_model.rs (1)

41-41: Ohayo! Parameter rename looks good, sensei!

The removal of the underscore prefix from _config is appropriate since the configuration is now actively used for namespace filtering.

crates/torii/core/src/processors/register_event.rs (2)

41-41: Ohayo! Parameter usage change looks good, sensei!

The removal of the underscore prefix from _config indicates this parameter is now actively used, which aligns perfectly with the new namespace filtering feature.


61-61: 🛠️ Refactor suggestion

Consider safer error handling for namespace conversion, sensei!

The namespace string conversion uses unwrap() which could panic. Consider using map_err() to provide a more graceful error handling approach.

-let namespace = event.namespace.to_string().unwrap();
+let namespace = event.namespace.to_string().map_err(|e| {
+    anyhow::anyhow!("Failed to convert namespace to string: {}", e)
+})?;

Likely invalid or redundant comment.

crates/torii/core/src/processors/store_update_member.rs (1)

59-69: Ohayo! The error handling looks good, sensei! Let's verify consistency.

The new error handling pattern that silently ignores missing models aligns well with the namespace-specific indexing feature. The implementation is clean and well-documented.

Let's check if this error handling pattern is consistent across other processors:

✅ Verification successful

Ohayo! The error handling is perfectly consistent across all processors, sensei!

The verification shows that all store-related processors (store_update_record.rs, store_set_record.rs, store_del_record.rs) follow the exact same error handling pattern for missing models, silently returning Ok(()) when encountering "no rows" errors. Even event_message.rs follows a similar pattern with a simplified implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check error handling patterns in other processors
# Expected: Similar error handling for "no rows" in other processor implementations

# Search for similar error handling patterns in other processors
ast-grep --pattern 'if $_.contains("no rows") {
  return Ok(());
}'

# Also check for any direct error propagation that might need similar handling
rg -A 2 'db\.model\(' crates/torii/core/src/processors/

Length of output: 2615

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 115913d and 8f807cc.

📒 Files selected for processing (3)
  • bin/torii/src/main.rs (2 hunks)
  • crates/torii/cli/src/args.rs (2 hunks)
  • crates/torii/cli/src/options.rs (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/torii/cli/src/args.rs
  • crates/torii/cli/src/options.rs
🔇 Additional comments (2)
bin/torii/src/main.rs (2)

126-126: Ohayo! Clean naming update looks good, sensei!

The simplified flag naming from index_transactions to transactions maintains functionality while improving readability.


143-143: Verify historical events handling, sensei!

The removal of unwrap_or_default() suggests that args.events.historical is now guaranteed to be initialized. Let's verify this assumption:

Also applies to: 147-147

✅ Verification successful

Ohayo! The historical events handling is safe, sensei!

The removal of unwrap_or_default() is perfectly fine because:

  • historical is a Vec<String> with a default implementation (vec![])
  • It's properly configured with clap's arg attribute for CLI parsing
  • The struct implements Default trait returning an empty vector

The direct into_iter() usage is safe since we'll always have at least an empty vector to iterate over.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for EventsOptions struct definition and usage
rg -A 5 "struct EventsOptions"

# Search for historical field initialization
rg "historical.*Vec<String>"

Length of output: 603


Script:

#!/bin/bash
# Let's check the field type and initialization in the CLI args
ast-grep --pattern 'historical: Vec<String>'

# Check if there's a default value or builder pattern
rg -B 3 'historical.*=.*Vec'

# Look for any clap/arg attributes related to historical
rg -B 2 '#\[arg.*historical\]'

Length of output: 110


Script:

#!/bin/bash
# Let's check how historical is used in the codebase
rg -A 2 "historical" crates/torii/cli/src/options.rs

# Check for any clap/arg attributes with a more flexible pattern
rg "#\[arg.*historical.*\]" crates/torii/cli/src/options.rs

# Look for the struct field and its attributes
rg -B 3 "pub historical:" crates/torii/cli/src/options.rs

Length of output: 724

polling_interval: Duration::from_millis(args.indexing.polling_interval),
flags,
event_processor_config: EventProcessorConfig {
historical_events: args.events.historical.unwrap_or_default().into_iter().collect(),
historical_events: args.events.historical.into_iter().collect(),
namespaces: args.indexing.namespaces.into_iter().collect(),
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo! Documentation needed for the new namespaces feature, sensei!

The addition of namespace filtering is a significant feature that deserves proper documentation. Please consider:

  1. Adding documentation comments explaining the purpose and usage of namespaces
  2. Including example configurations in the README
  3. Adding tests to verify namespace filtering behavior

Would you like me to help draft the documentation and test cases for this new feature?

@Larkooo Larkooo enabled auto-merge (squash) November 14, 2024 06:04
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
crates/torii/cli/src/args.rs (1)

286-292: LGTM! Consider adding edge case tests

Ohayo sensei! The test assertions have been properly updated to reflect the new structure. However, consider adding test cases for:

  • Non-empty namespaces configuration
  • Invalid namespace formats (if any restrictions exist)
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dcb3ee5 and 9d672e1.

📒 Files selected for processing (1)
  • crates/torii/cli/src/args.rs (3 hunks)
🔇 Additional comments (2)
crates/torii/cli/src/args.rs (2)

239-239: LGTM! Test assertion correctly updated

Ohayo! The test assertion has been properly updated to reflect the removal of the Option wrapper for historical events.


264-271: Verify the empty namespaces default value

Ohayo sensei! While the configuration changes look good, we should verify if an empty array is the intended default for the new namespaces field. This could affect the indexing behavior if no namespaces are specified.

Let's check if there's any documentation or related code that specifies the expected behavior:

✅ Verification successful

Empty namespaces array is the correct default

Ohayo sensei! After checking the codebase, I can confirm that an empty namespaces array is indeed the correct default. The NamespaceConfig implementation shows that when no explicit namespaces are provided, the system falls back to using a default namespace ("dojo"), as evidenced by the test_get_namespaces_default test. This means the empty array in the configuration is valid and won't affect indexing behavior negatively.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for documentation or usage of the namespaces field
rg -g '!target' -g '*.{rs,md,toml}' -B 2 -A 2 'namespaces.*=.*\[\]' 

# Search for any comments or documentation about namespaces behavior
rg -g '!target' -g '*.{rs,md}' -B 3 -A 3 'namespaces.*field'

Length of output: 1112


Script:

#!/bin/bash
# Let's search for the struct definition and documentation of the namespaces field
ast-grep --pattern 'struct $_ {
  $$$
  namespaces: $_,
  $$$
}'

# Search for any code that processes or validates the namespaces field
rg -g '!target' 'fn.*namespaces' -B 2 -A 5

# Search for any tests related to namespaces configuration
rg -g '!target' -g '*.rs' 'test.*namespaces' -B 2 -A 5

Length of output: 10253

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 67.10526% with 25 lines in your changes missing coverage. Please review.

Project coverage is 57.25%. Comparing base (4671d9c) to head (9d672e1).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...s/torii/core/src/processors/store_update_member.rs 0.00% 12 Missing ⚠️
bin/torii/src/main.rs 0.00% 4 Missing ⚠️
crates/torii/cli/src/options.rs 62.50% 3 Missing ⚠️
...ates/torii/core/src/processors/store_del_record.rs 81.81% 2 Missing ⚠️
...ates/torii/core/src/processors/store_set_record.rs 81.81% 2 Missing ⚠️
...s/torii/core/src/processors/store_update_record.rs 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2687      +/-   ##
==========================================
- Coverage   57.41%   57.25%   -0.16%     
==========================================
  Files         406      408       +2     
  Lines       51330    51754     +424     
==========================================
+ Hits        29470    29631     +161     
- Misses      21860    22123     +263     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants