Skip to content

fix: builder methods return Result instead of panicking on validation #1643#1644

Open
workonlly wants to merge 3 commits into
mofa-org:mainfrom
workonlly:pullrequest_!
Open

fix: builder methods return Result instead of panicking on validation #1643#1644
workonlly wants to merge 3 commits into
mofa-org:mainfrom
workonlly:pullrequest_!

Conversation

@workonlly
Copy link
Copy Markdown

📋 Summary
This PR refactors the builder pattern implementation in mofa-foundation to replace panic!() calls with proper Result returns during validation. This ensures that the library fails gracefully and adheres to the project's error-handling standards.

🔗 Related Issues
Closes #1

Related to #1643

🧠 Context
Previously, the .build() methods for AdapterDescriptor and ModelConfig would crash the entire application if required fields (like IDs or names) were missing. This violated CLAUDE.md Section VI.2, which mandates that builders must validate input and return a Result.

By returning AdapterError::InvalidConfig, we allow consuming applications to handle configuration errors programmatically rather than suffering unrecoverable crashes.

🛠️ Changes
API Refactor: Updated .build() signature for AdapterDescriptorBuilder and ModelConfigBuilder to return Result<T, AdapterError>.

Validation Logic: Replaced 5 panic!() instances with structured error propagation.

Call Site Migration: Updated 10 locations in the test suite and internal modules to handle the new Result type.

Documentation: Updated doc comments and module-level examples to reflect the new fallible API and the use of the ? operator.

🧪 How you Tested
Unit Tests: Ran the adapter test suite to ensure existing logic remains sound.

Bash
cargo test -p mofa-foundation adapter
Regression Testing: Verified that all 61 existing tests pass with the new return types.

Error Path Verification: Added 5 new test cases specifically designed to trigger and catch the InvalidConfig error variants when mandatory fields are omitted.

Screenshot 2026-04-19 091402

⚠️ Breaking Changes
[ ] No breaking changes

[x] Breaking change (describe below)

Impact: Any code calling .build() on AdapterDescriptorBuilder or ModelConfigBuilder will now receive a Result. Users will need to add .unwrap(), .expect(), or use the ? operator to access the built object.

🧹 Checklist
Code Quality
[x] Code follows Rust idioms and project conventions

[x] cargo fmt run

[x] cargo clippy passes without warnings

Testing
[x] Tests added/updated

[x] cargo test passes locally without any error

Documentation
[x] Public APIs documented

[x] README / docs updated (if needed)

PR Hygiene
[x] PR is small and focused (one logical change)

[x] Branch is up to date with main

[x] No unrelated commits

[x] Commit messages explain why, not only what

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.

Creating a node hub to be able to reference the node that is being used

1 participant