feat(minibf): add optional base_path configuration#872
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds optional ChangesBase path prefix configuration and routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/core/src/config.rs`:
- Around line 572-575: Update the doc comment for the `base_path:
Option<String>` field in crates/core/src/config.rs to reflect the actual
behavior in `build_router()` (lib.rs): when `base_path` is set, all
routes—including `/health` and `/metrics`—are nested under that base path;
remove or correct the line that says "/health and /metrics remain at root" so
the comment matches the implementation (or, if you prefer the other approach,
modify `build_router()` to exclude `/health` and `/metrics` from nesting under
`base_path`).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/minibf/src/lib.rs`:
- Around line 442-446: The config validation for cfg.base_path is too narrow and
must also reject values missing a leading '/' (and optionally containing
wildcards) to avoid Axum's opaque panic when calling
Router::new().nest(base_path, app); update the check around cfg.base_path in
lib.rs to: ensure base_path is not empty, starts with '/', is not just "/" (as
already done), and does not contain '*' (or other invalid characters you choose
to forbid); if validation fails, panic or return a clear error message
referencing cfg.base_path so callers see a helpful configuration error before
reaching Router::nest (which is where NormalizePathLayer::trim_trailing_slash()
is applied).
| panic!( | ||
| "base_path must start with '/', must not be just '/', and must not contain wildcards; got: \"{}\"", | ||
| base_path |
There was a problem hiding this comment.
we should avoid a panic and instead bubble up an error, ideally a "config" error variant.
There was a problem hiding this comment.
I've addressed the panic by adding a ConfigError(String) variant to ServeError in dolos-core and changing build_router to return Result<Router, ServeError>. Invalid base_path values now return Err(ServeError::ConfigError(...)) instead of panicking, propagating through Driver::run consistently with BindError and ShutdownError.
I've also rebased onto the current main to resolve the CI conflicts.
4da0643 to
f1a5f18
Compare
|
Thanks for this — Blockfrost |
cdfa20b to
b8f9a7b
Compare
|
Rebased onto current The inline On your open question — agreed that opt-in is the right call for now. Defaulting to |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/minibf/src/test_support.rs (1)
126-126: ⚡ Quick winAdd a regression test for the new
base_pathcontract.This helper now only unwraps the fallible builder. Please add an integration case that builds with
base_path = Some("/api/v0".into())and asserts prefixed routes resolve while root routes 404, plus one invalid-path case that returnsServeError::ConfigError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minibf/src/test_support.rs` at line 126, Add an integration test exercising the new base_path contract since build_router_with_facade now unwraps a fallible builder: write a test that calls the fallible builder via build_router_with_facade(facade) with facade configured with base_path = Some("/api/v0".into()) and assert that routes under "/api/v0/..." resolve successfully while the same routes at root "/" return 404; also add a test case that supplies an invalid base_path and asserts the builder fails with ServeError::ConfigError. Locate and modify the tests around build_router_with_facade and the test helper in test_support.rs to create these two cases (valid prefixed routing and invalid-path config error) and assert the expected outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minibf/src/lib.rs`:
- Around line 512-528: The base_path validation allows values ending with '/'
which will not align with Router::nest because
NormalizePathLayer::trim_trailing_slash() normalizes requests but nest uses the
prefix literally; update the logic around base_path in the function that
constructs the Router so that you canonicalize by removing any trailing '/'
(e.g. rtrim) before calling Router::nest (or alternatively return
ServeError::ConfigError if you prefer to reject trailing slashes), ensuring the
resulting Router::new().nest(canonical_base_path, app) uses the trimmed value
while still applying NormalizePathLayer::trim_trailing_slash().
---
Nitpick comments:
In `@crates/minibf/src/test_support.rs`:
- Line 126: Add an integration test exercising the new base_path contract since
build_router_with_facade now unwraps a fallible builder: write a test that calls
the fallible builder via build_router_with_facade(facade) with facade configured
with base_path = Some("/api/v0".into()) and assert that routes under
"/api/v0/..." resolve successfully while the same routes at root "/" return 404;
also add a test case that supplies an invalid base_path and asserts the builder
fails with ServeError::ConfigError. Locate and modify the tests around
build_router_with_facade and the test helper in test_support.rs to create these
two cases (valid prefixed routing and invalid-path config error) and assert the
expected outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32802faf-1d2b-41d7-a521-1d5226132704
📒 Files selected for processing (7)
crates/core/src/config.rscrates/core/src/lib.rscrates/minibf/README.mdcrates/minibf/src/lib.rscrates/minibf/src/test_support.rsdocs/content/apis/minibf.mdxsrc/bin/dolos/minibf.rs
✅ Files skipped from review due to trivial changes (3)
- src/bin/dolos/minibf.rs
- crates/minibf/README.md
- docs/content/apis/minibf.mdx
Adds an optional `base_path` field to `MinibfConfig` that nests all minibf routes (including `/health` and `/metrics`) under a configurable path prefix. Set to `/api/v0` for full Blockfrost OpenAPI compliance; when omitted, routes remain at the root (no behavioral change). Invalid values (empty, just `/`, missing leading `/`, or containing `*`) return a new `ServeError::ConfigError` variant instead of panicking.
b8f9a7b to
8f61088
Compare
|
Addressed both coderabbit findings in
All 214 minibf tests pass. |
Add optional base_path configuration for Blockfrost API spec compliance
Problem
The Blockfrost OpenAPI specification defines all API endpoints under the
/api/v0base path (seeserverssection in the official spec). However, Dolos minibf currently serves endpoints directly at the root level (e.g.,/network,/blocks/latest), which breaks compatibility with tools and libraries that expect canonical Blockfrost endpoint paths.This incompatibility causes integration issues with:
/api/v0base pathSolution
Add an optional
base_pathconfiguration field toMinibfConfigthat allows nesting all routes under a custom prefix.Configuration example:
When configured:
{base_path}/{endpoint}(e.g.,/api/v0/network)Changes
base_path: Option<String>field toMinibfConfigbuild_router()to conditionally nest all routes whenbase_pathis set (~7 lines)base_pathto default configuration initializationImplementation Details
The implementation uses Axum's
Router::nest()to wrap the entire route tree under the custom base path when configured:This approach minimizes code changes while providing complete Blockfrost spec compliance.
Backwards Compatibility
base_pathis not configured, all routes remain at root level exactly as before.Testing
cargo test --workspace --all-features)cargo clippy --workspace --all-features)/api/v0base path/api/v0/*with base_path configuredImpact
This change enables Dolos minibf to serve as a drop-in replacement for Blockfrost API in tools requiring canonical endpoint paths, significantly improving ecosystem compatibility while maintaining flexibility for users who prefer root-level routes.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Improvements