fix: builder methods return Result instead of panicking on validation #1643#1644
Open
workonlly wants to merge 3 commits into
Open
fix: builder methods return Result instead of panicking on validation #1643#1644workonlly wants to merge 3 commits into
workonlly wants to merge 3 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 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.
[ ] 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