refactor: entity descriptors #2047
Conversation
WalkthroughThese changes introduce a comprehensive descriptor system for components, endpoints, and namespaces, replacing raw string-based identities with validated, structured descriptor types. New modules and types are added for entity descriptors, and existing structs are refactored to use these descriptors for identity, equality, and metadata. Several new methods and helper functions are introduced, and a local runtime registry is implemented for endpoint engines. The runtime's unique identifier is removed, and internal logic is updated to consistently use descriptor-derived information. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LocalRuntime
participant Registry
participant Engine
User->>LocalRuntime: register_engine(descriptor, engine)
LocalRuntime->>Registry: Lock and insert (descriptor, engine)
Registry-->>LocalRuntime: Success/Error
LocalRuntime-->>User: Result
User->>LocalRuntime: get_engine(descriptor)
LocalRuntime->>Registry: Lock and lookup descriptor
Registry-->>LocalRuntime: Option<engine>
LocalRuntime-->>User: Option<engine>
sequenceDiagram
participant Component
participant Descriptor
participant Endpoint
Component->>Descriptor: from_parts(namespace_segments, component_name)
Component-->>Component: Stores ComponentDescriptor
Component->>Endpoint: endpoint(endpoint_name)
Endpoint->>Descriptor: from_parts(namespace_segments, component_name, endpoint_name)
Endpoint-->>Endpoint: Stores EndpointDescriptor
Estimated code review effort5 (~150 minutes) Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lib/runtime/src/component.rs (1)
777-786: Remove unusedvalidate_allowed_charsincomponent.rsThis private helper is never called within its module (and is shadowed by a separate implementation in
transports/etcd/path.rs), so it can be removed as dead code.
- lib/runtime/src/component.rs (around lines 777–786): delete the entire
validate_allowed_charsfunction.- fn validate_allowed_chars(input: &str) -> Result<(), ValidationError> { - // Define the allowed character set using a regex - let regex = regex::Regex::new(r"^[a-z0-9-_]+$").unwrap(); - - if regex.is_match(input) { - Ok(()) - } else { - Err(ValidationError::new("invalid_characters")) - } - }
🧹 Nitpick comments (3)
lib/runtime/src/discovery.rs (1)
89-93: Consider adding documentation or making the trait private.The
DiscoveryPlanetrait is completely empty, which can be confusing for users. Consider either:
- Adding documentation explaining its intended purpose and future functionality
- Making it
pub(crate)or private until it has actual methods to prevent premature public API exposure+/// Placeholder trait for future discovery functionality. +/// This trait will be expanded with discovery-related methods in upcoming releases. pub trait DiscoveryPlane {lib/runtime/src/entity/descriptor.rs (1)
475-486: Consider extracting path segment parsing for clarity.The path segment parsing logic works correctly but could be more explicit:
PATH_KEYWORD => { i += 1; if i >= segments.len() { return Err(DescriptorError::InvalidUrl( "Path keyword requires at least one path segment".to_string(), )); } + // Collect all remaining segments as path while i < segments.len() { path_segments.push(segments[i].to_string()); i += 1; } }lib/runtime/src/component.rs (1)
199-201: Consider inlining the helper function call.Since
descriptor.inner().component()returns anOption<&str>, you could simplify this to:pub fn name(&self) -> String { - extract_name_or_empty(self.descriptor.inner().component()) + self.descriptor.inner().component().unwrap_or("").to_string() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/runtime/src/component.rs(12 hunks)lib/runtime/src/component/component.rs(1 hunks)lib/runtime/src/component/endpoint.rs(1 hunks)lib/runtime/src/component/namespace.rs(1 hunks)lib/runtime/src/component/service.rs(1 hunks)lib/runtime/src/discovery.rs(1 hunks)lib/runtime/src/entity.rs(1 hunks)lib/runtime/src/entity/descriptor.rs(1 hunks)lib/runtime/src/lib.rs(2 hunks)lib/runtime/src/local.rs(1 hunks)lib/runtime/src/runtime.rs(0 hunks)
🧠 Learnings (7)
lib/runtime/src/component/service.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/discovery.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/component/namespace.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/entity.rs (2)
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/component/component.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/lib.rs (2)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
lib/runtime/src/local.rs (2)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
💤 Files with no reviewable changes (1)
- lib/runtime/src/runtime.rs
🧰 Additional context used
🧠 Learnings (7)
lib/runtime/src/component/service.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/discovery.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/component/namespace.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/entity.rs (2)
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/component/component.rs (1)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
lib/runtime/src/lib.rs (2)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
lib/runtime/src/local.rs (2)
Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
🔇 Additional comments (23)
lib/runtime/src/component/endpoint.rs (1)
119-121: LGTM! Proper migration to descriptor-based name access.The changes correctly replace direct field access with method calls, aligning with the descriptor system refactor. This improves encapsulation by using the descriptor as the source of truth for component and namespace names.
lib/runtime/src/entity.rs (1)
1-5: LGTM! Clean module declaration.The file properly declares the public descriptor module with appropriate SPDX license headers. This provides a clean entry point for the new descriptor system.
lib/runtime/src/component/namespace.rs (1)
28-28: LGTM! Consistent descriptor-based name access.The change properly replaces direct field access with the
name()method call, maintaining consistency with the descriptor system refactor throughout the codebase.lib/runtime/src/component/component.rs (1)
27-33: LGTM! Consistent with descriptor-based refactoring.The change from direct field access to method calls (
name()) aligns with the broader refactoring to use descriptor types for identity and metadata. This improves encapsulation and makes the code consistent with the new architecture.lib/runtime/src/lib.rs (2)
39-39: LGTM! New modules properly integrated.The addition of
entityandlocalmodules aligns with the PR's introduction of the descriptor system and local runtime management.Also applies to: 41-41
78-89: Well-designed LocalRuntime struct.The
LocalRuntimestruct appropriately:
- Wraps the existing
Runtimefor local resource management- Uses
std::sync::Mutexfor the registry (appropriate for quick operations)- Employs
Arcfor thread-safe sharing of the registry- Uses the new
EndpointDescriptoras keys, maintaining type safetylib/runtime/src/entity/descriptor.rs (7)
33-113: Well-designed InstanceType with proper validation.The
InstanceTypeenum correctly:
- Enforces positive IDs for distributed instances (matching etcd lease requirements)
- Uses lowercase hex formatting consistently
- Provides clear error messages
- Handles the "local" string case appropriately
115-144: Comprehensive error handling design.The
DescriptorErrorenum provides excellent coverage of validation scenarios with clear, actionable error messages. Good use of thiserror for the implementation.
146-296: Solid EntityDescriptor core implementation.The design correctly enforces the entity hierarchy (endpoint requires component, instance requires endpoint) and provides comprehensive validation. The dual format support is well-structured.
524-1215: Excellent type-safe descriptor hierarchy.The specialized descriptor types provide:
- Strong compile-time guarantees about entity relationships
- Optimized transitions that only validate new fields
- Clear separation between user namespaces and the internal namespace
- Comprehensive conversion implementations
This is a well-thought-out design that prevents misuse while maintaining performance.
1217-1327: Well-implemented builder pattern.The builder provides good ergonomics with:
- Clear method chaining
- Both validated and unchecked build options (useful for performance)
- Convenient From trait for pre-validated data
1329-1393: Proper validation with clear rules.The validation helpers correctly:
- Reserve underscore prefixes for internal use (except in namespace segments)
- Enforce consistent naming rules (lowercase alphanumeric + underscore)
- Provide detailed error messages with context
1395-2573: Exemplary test coverage!The test suite is comprehensive and well-organized, covering:
- All parsing formats and edge cases
- Validation boundaries and error conditions
- Type conversions and transitions
- Round-trip consistency
- Special cases like the internal namespace
This level of testing provides excellent confidence in the descriptor system's correctness.
lib/runtime/src/component.rs (10)
32-40: Import changes look good!The new descriptor imports align well with the refactoring objectives to use structured descriptor types instead of raw strings.
135-138: Good encapsulation with the descriptor field!The descriptor field is correctly marked as private in the builder pattern, ensuring it's managed internally.
140-146: Hash implementation correctly uses descriptor data.Good use of the descriptor's namespace segments and component fields for hashing.
208-224: Well-structured endpoint creation method!The method properly creates an endpoint descriptor from the component descriptor and validates the endpoint name.
335-337: Good descriptor integration in Endpoint struct!The descriptor serves as the primary source of truth while maintaining backward compatibility.
375-378: Clear backward compatibility approach.Good to maintain the stored name for compatibility while the descriptor becomes the source of truth.
536-556: Well-designed instance descriptor creation!The method correctly differentiates between static and dynamic endpoints, using appropriate instance types for each case.
580-583: Clean descriptor integration in Namespace!The descriptor pattern is consistently applied across all three core structs.
628-648: Elegant parent namespace implementation!The method correctly handles the hierarchical namespace structure by creating parent namespaces on-demand.
716-774: Excellent helper function organization!These helper functions effectively reduce code duplication and improve maintainability. The error handling helper
descriptor_error()provides consistent error messages across the codebase.
| component.name(), | ||
| component.namespace | ||
| )); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency in namespace access pattern.
The change to component.name() correctly follows the descriptor-based approach. However, line 55 still uses direct field access component.namespace while other files in this refactor use method calls. Please verify this is intentional or if it should be component.namespace.name() for consistency.
🏁 Script executed:
#!/bin/bash
# Description: Check how namespace names are accessed in the codebase after the descriptor refactor
# Expected: Should see consistent usage pattern for namespace name access
# Search for namespace name access patterns
rg -A 2 -B 2 "\.namespace\." --type rust
rg -A 2 -B 2 "namespace\.name\(\)" --type rustLength of output: 5273
🏁 Script executed:
#!/bin/bash
# Show context where component.namespace is used in service.rs
rg -n -C 4 "component.namespace" lib/runtime/src/component/service.rs
# Show struct Service definition in that file
rg -n -C 4 "struct Service" lib/runtime/src/component/service.rsLength of output: 745
🏁 Script executed:
#!/bin/bash
# Locate the definition of struct Component to inspect namespace field type
rg -n "pub struct Component" -A6 lib/runtime/src/component.rs
# Check for a struct or type definition for Namespace
rg -n "pub struct Namespace" -A6 lib/runtime/src
# Check if Namespace implements Display
rg -n "impl std::fmt::Display for Namespace" -A6 lib/runtime/srcLength of output: 1770
Use explicit namespace.name() for consistency in service description
The codebase uniformly calls namespace.name() rather than relying on the Display impl when formatting namespace names. Please update the service description to match this pattern.
• lib/runtime/src/component/service.rs:55
- component.namespace
+ component.namespace.name()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| component.name(), | |
| component.namespace | |
| )); | |
| component.name(), | |
| component.namespace.name() | |
| )); |
🤖 Prompt for AI Agents
In lib/runtime/src/component/service.rs around lines 54 to 56, the service
description formats the namespace using the Display trait directly. Update the
code to explicitly call namespace.name() instead of relying on the Display
implementation to maintain consistency with the rest of the codebase.
| impl LocalRuntime { | ||
| /// Create a new [LocalRuntime] with the given [Runtime]. | ||
| pub fn new(runtime: Runtime) -> anyhow::Result<LocalRuntime> { | ||
| Ok(LocalRuntime { | ||
| runtime, | ||
| endpoint_engines: Arc::new(Mutex::new(HashMap::new())), | ||
| }) | ||
| } | ||
|
|
||
| /// Get an [AnyAsyncEngine] for an [EndpointDescriptor] if it exists, otherwise return None. | ||
| pub fn get_engine(&self, descriptor: &EndpointDescriptor) -> Option<Arc<dyn AnyAsyncEngine>> { | ||
| let engines = self.endpoint_engines.lock().unwrap(); | ||
| engines.get(descriptor).cloned() | ||
| } | ||
|
|
||
| /// Register an [AnyAsyncEngine] to an [EndpointDescriptor]. If an engine is already | ||
| /// registered for the descriptor an error is returned. | ||
| pub(crate) fn register_engine( | ||
| &self, | ||
| descriptor: EndpointDescriptor, | ||
| engine: Arc<dyn AnyAsyncEngine>, | ||
| ) -> anyhow::Result<()> { | ||
| let mut engines = self.endpoint_engines.lock().unwrap(); | ||
|
|
||
| if engines.contains_key(&descriptor) { | ||
| return Err(anyhow::anyhow!("Engine already registered for descriptor")); | ||
| } | ||
|
|
||
| engines.insert(descriptor, engine); | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle mutex poisoning instead of using unwrap().
The implementation looks good overall, but both get_engine() and register_engine() use .unwrap() on mutex locks. This could cause a panic if the mutex is poisoned (when another thread panics while holding the lock).
Consider handling the potential error:
pub fn get_engine(&self, descriptor: &EndpointDescriptor) -> Option<Arc<dyn AnyAsyncEngine>> {
- let engines = self.endpoint_engines.lock().unwrap();
+ let engines = self.endpoint_engines.lock()
+ .map_err(|e| anyhow::anyhow!("Mutex poisoned: {}", e))?;
engines.get(descriptor).cloned()
}However, since get_engine returns Option rather than Result, you might want to:
pub fn get_engine(&self, descriptor: &EndpointDescriptor) -> Option<Arc<dyn AnyAsyncEngine>> {
- let engines = self.endpoint_engines.lock().unwrap();
+ let engines = match self.endpoint_engines.lock() {
+ Ok(guard) => guard,
+ Err(_) => return None, // Or log the error and return None
+ };
engines.get(descriptor).cloned()
}Similarly for register_engine:
pub(crate) fn register_engine(
&self,
descriptor: EndpointDescriptor,
engine: Arc<dyn AnyAsyncEngine>,
) -> anyhow::Result<()> {
- let mut engines = self.endpoint_engines.lock().unwrap();
+ let mut engines = self.endpoint_engines.lock()
+ .map_err(|e| anyhow::anyhow!("Mutex poisoned: {}", e))?;
if engines.contains_key(&descriptor) {
return Err(anyhow::anyhow!("Engine already registered for descriptor"));
}
engines.insert(descriptor, engine);
Ok(())
}🤖 Prompt for AI Agents
In lib/runtime/src/local.rs from lines 14 to 46, the use of unwrap() on the
mutex lock in get_engine() and register_engine() can cause panics if the mutex
is poisoned. To fix this, handle the Result returned by lock() instead of
unwrapping. For get_engine(), return None if the lock is poisoned by using match
or if let to handle the error case gracefully. For register_engine(), propagate
the error by converting the poisoned lock error into an anyhow::Error and
returning it as a Result. This ensures safe handling of mutex poisoning without
panics.
|
|
||
|
|
||
|
|
||
| } |
ryanolson
added
backlog
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
Summary by CodeRabbit
New Features
Refactor
Chores