Use registry-provided token list URL instead of hardcoded constant#59
Open
Use registry-provided token list URL instead of hardcoded constant#59
Conversation
Pick up the commit that exposes `get_remote_tokens()` from `RaindexClient`, which parses the `using-tokens-from` field in the registry YAML.
Replace the hardcoded TOKEN_LIST_URL and TokensConfig fairing with a dynamic lookup via RaindexProvider.get_remote_token_urls(), which reads the using-tokens-from field from the registry YAML. The handler now takes SharedRaindexProvider as a Rocket state parameter, resolves the first configured URL at request time, and fetches the token list with a fresh reqwest client.
Remove token_list_url from TestClientBuilder and mock_token_list_url(). Instead, mock_raindex_registry_url() now accepts an optional token URL and embeds it as using-tokens-from in the mock registry YAML, so tests exercise the full registry-based URL resolution path.
📝 WalkthroughWalkthroughThe changes refactor token URL retrieval from a configuration-based approach (TokensConfig fairing) to a registry-backed approach using SharedRaindexProvider. This includes removing the tokens fairing from Rocket initialization, adding a new method to fetch remote token URLs, and updating token routes and test infrastructure accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as GET /tokens Handler
participant Provider as SharedRaindexProvider
participant Registry as Raindex Registry Client
participant Remote as Remote Token Service
Client->>Handler: GET /tokens
Handler->>Provider: get_remote_token_urls()
Provider->>Registry: initialize client from registry
Registry-->>Provider: client ready
Provider->>Registry: get_remote_tokens()
Registry-->>Provider: token configs with urls
Provider->>Provider: extract & collect URLs
Provider-->>Handler: Vec<String> (urls)
Handler->>Handler: select first URL or error
Handler->>Remote: HTTP GET token list
Remote-->>Handler: token list JSON
Handler->>Handler: deserialize & respond
Handler-->>Client: 200 OK with tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/routes/tokens.rs (1)
130-136: Add a regression test for the missing-token-URL path.The new
NoTokenListUrlbranch is introduced but not covered in this test module. Add one test that usesmock_raindex_registry_url(None)and asserts the expected API error contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/tokens.rs` around lines 130 - 136, Add a regression test to cover the NoTokenListUrl branch by creating a client with mock_raindex_registry_url(None) instead of Some(token_url): write a new async test (in the same module) that calls build_client_with_token_url-equivalent setup but passes None to mock_raindex_registry_url, uses TestClientBuilder::new().raindex_registry_url(registry_url).build().await to obtain the client, invokes the relevant endpoint (same API route exercised by other tests in this file), and asserts the API returns the expected error contract for missing token URL (status code and JSON error shape) to verify the NoTokenListUrl path is exercised.src/test_helpers.rs (1)
87-89: Quote the injected token URL in YAML for robustness.Unquoted URLs can become fragile with special characters (for example
#), so quoting reduces parser edge cases in tests.🧪 Optional hardening diff
- Some(url) => format!("\nusing-tokens-from:\n - {url}\n"), + Some(url) => format!("\nusing-tokens-from:\n - \"{url}\"\n"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test_helpers.rs` around lines 87 - 89, The YAML fragment built in the match for token_list_url (the using_tokens variable) must quote the injected URL to avoid parser issues with characters like #; change the Some(url) branch to wrap the URL in double quotes (e.g. " {url} ") and escape any embedded quotes in url (url.replace("\"", "\\\"")) before formatting so the produced YAML is robust against special characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/tokens.rs`:
- Around line 52-55: The registry URL resolution using
shared_raindex.read().await and raindex.get_remote_token_urls() which can return
TokenError::NoTokenListUrl must be moved inside the instrumented/request-scoped
block that logs "request received" so tracing always records the request before
any fallible operations; locate the code referencing shared_raindex and
get_remote_token_urls() in tokens.rs and move it into the block where the
request is instrumented/logged (the area around the "request received" tracing
call), ensure you drop the read guard after use, and keep error handling
(TokenError::NoTokenListUrl) unchanged but after the initial log so
request-level tracing is always emitted.
---
Nitpick comments:
In `@src/routes/tokens.rs`:
- Around line 130-136: Add a regression test to cover the NoTokenListUrl branch
by creating a client with mock_raindex_registry_url(None) instead of
Some(token_url): write a new async test (in the same module) that calls
build_client_with_token_url-equivalent setup but passes None to
mock_raindex_registry_url, uses
TestClientBuilder::new().raindex_registry_url(registry_url).build().await to
obtain the client, invokes the relevant endpoint (same API route exercised by
other tests in this file), and asserts the API returns the expected error
contract for missing token URL (status code and JSON error shape) to verify the
NoTokenListUrl path is exercised.
In `@src/test_helpers.rs`:
- Around line 87-89: The YAML fragment built in the match for token_list_url
(the using_tokens variable) must quote the injected URL to avoid parser issues
with characters like #; change the Some(url) branch to wrap the URL in double
quotes (e.g. " {url} ") and escape any embedded quotes in url (url.replace("\"",
"\\\"")) before formatting so the produced YAML is robust against special
characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c51c3557-f21c-4cbb-8c2a-26fa927cca62
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/rain.orderbooksrc/main.rssrc/raindex/config.rssrc/routes/admin.rssrc/routes/tokens.rssrc/test_helpers.rs
💤 Files with no reviewable changes (1)
- src/main.rs
Comment on lines
+52
to
+55
| let raindex = shared_raindex.read().await; | ||
| let urls = raindex.get_remote_token_urls()?; | ||
| let url = urls.first().ok_or(TokenError::NoTokenListUrl)?.to_string(); | ||
| drop(raindex); |
There was a problem hiding this comment.
Ensure request logging happens before fallible registry URL resolution.
Line [52]-Line [55] can fail before the "request received" log at Line [58], so some requests may return without required request-level tracing. Move URL resolution inside the instrumented block after initial request logging.
🔧 Proposed fix
pub async fn get_tokens(
_global: GlobalRateLimit,
_key: AuthenticatedKey,
span: TracingSpan,
shared_raindex: &State<SharedRaindexProvider>,
) -> Result<Json<TokenListResponse>, ApiError> {
- let raindex = shared_raindex.read().await;
- let urls = raindex.get_remote_token_urls()?;
- let url = urls.first().ok_or(TokenError::NoTokenListUrl)?.to_string();
- drop(raindex);
-
async move {
tracing::info!("request received");
+ let url = {
+ let raindex = shared_raindex.read().await;
+ let urls = raindex.get_remote_token_urls()?;
+ urls.first().cloned().ok_or_else(|| {
+ tracing::warn!("no token list URL configured in registry");
+ TokenError::NoTokenListUrl
+ })?
+ };
tracing::info!(url = %url, timeout_secs = TOKEN_LIST_TIMEOUT_SECS, "fetching token list");Also applies to: 57-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/tokens.rs` around lines 52 - 55, The registry URL resolution using
shared_raindex.read().await and raindex.get_remote_token_urls() which can return
TokenError::NoTokenListUrl must be moved inside the instrumented/request-scoped
block that logs "request received" so tracing always records the request before
any fallible operations; locate the code referencing shared_raindex and
get_remote_token_urls() in tokens.rs and move it into the block where the
request is instrumented/logged (the area around the "request received" tracing
call), ensure you drop the read guard after use, and keep error handling
(TokenError::NoTokenListUrl) unchanged but after the initial log so
request-level tracing is always emitted.
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
1 participant
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.
Dependent PRs
Motivation
The token list URL was hardcoded as a constant in
src/routes/tokens.rs, pointing to a specific GitHub raw file commit. This made it impossible to change the token list source without redeploying the API. Now thatRaindexClientexposesget_remote_tokens()(PR #2490), the URL can be read from the registry YAML'susing-tokens-fromfield, making it configurable via the registry.Solution
rain.orderbooksubmodule to4df6e7dfbto pick up theget_remote_tokens()APIget_remote_token_urls()toRaindexProviderwhich delegates to the submodule'sRaindexClient::get_remote_tokens()to extract URLs from the registry YAMLget_tokenshandler to resolve the token list URL fromSharedRaindexProviderat request time instead of using a hardcoded constantTOKEN_LIST_URLconstant,TokensConfigstruct, its fairing, and the fairing attachment inmain.rstoken_list_urlbuilder method andmock_token_list_url();mock_raindex_registry_url()now accepts an optional token URL to embed asusing-tokens-fromin the mock registry YAML, so tests exercise the full registry-based resolution pathChecks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Bug Fixes
Chores