-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(indexer): new graphql url config option #1303
Conversation
WalkthroughThe changes introduce support for a GraphQL API endpoint by adding new configuration fields and updating existing documentation. The HTTP UI server now accepts a GraphQL address and returns it via a dedicated endpoint. Additionally, the indexer startup flow and swarm daemon components have been updated to allocate ports, include command parameters, and generate public URLs for GraphQL. The web UI now dynamically retrieves the GraphQL address, replacing a hardcoded value. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Server
Client->>HTTP_Server: GET /graphql_address
HTTP_Server-->>Client: Returns graphql_address string
sequenceDiagram
participant Indexer
participant EventManager
participant Config
Indexer->>Config: Read public JSON RPC & GraphQL addresses
Config-->>Indexer: Return addresses
Indexer->>EventManager: Instantiate with dependency components
alt Both addresses available
Indexer->>Indexer: Start GraphQL API and JSON-RPC API
else Address missing
Indexer->>Indexer: Skip API startup
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (4)
applications/tari_indexer_web_ui/src/utils/graphql.tsx (1)
1-1
: Fix the copyright year.The copyright year is set to 2025, which is incorrect as it's beyond the current year.
-// Copyright 2025. The Tari Project +// Copyright 2024. The Tari Projectapplications/tari_indexer/src/config.rs (1)
89-91
: Fix typo in GraphQL URL documentation.The documentation contains a typo: "jrpc" should be "GraphQL" since this is referring to the GraphQL API.
- /// The jrpc address where the UI should connect to the GraphQL API(it can be the same as the json_rpc_address, but doesn't have to + /// The GraphQL address where the UI should connect to the GraphQL API (it can be the same as the graphql_address, but doesn't have toapplications/tari_indexer_web_ui/src/routes/Events/Events.tsx (1)
62-64
: Consider removing debug console.log.The console.log statement might be useful during development but should be removed or wrapped in a debug flag for production.
let indexer_address = (await getGraphQLAddress()).toString(); - console.log({indexer_address});
applications/tari_indexer/src/lib.rs (1)
190-191
: Consider adding error handling for the HTTP UI server.The HTTP UI server is spawned without any error handling. Consider adding error handling to log any startup issues.
Apply this diff to add error handling:
- task::spawn(run_http_ui_server(address, public_jrpc_address, public_graphql_address)); + task::spawn({ + let address = address.clone(); + async move { + if let Err(e) = run_http_ui_server(address, public_jrpc_address, public_graphql_address).await { + error!(target: LOG_TARGET, "Failed to start HTTP UI server: {}", e); + } + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
applications/tari_indexer/src/cli.rs
(2 hunks)applications/tari_indexer/src/config.rs
(2 hunks)applications/tari_indexer/src/http_ui/server.rs
(1 hunks)applications/tari_indexer/src/lib.rs
(3 hunks)applications/tari_indexer_web_ui/src/routes/Events/Events.tsx
(2 hunks)applications/tari_indexer_web_ui/src/utils/graphql.tsx
(1 hunks)applications/tari_swarm_daemon/src/process_definitions/context.rs
(1 hunks)applications/tari_swarm_daemon/src/process_definitions/indexer.rs
(2 hunks)applications/tari_swarm_daemon/src/process_manager/handle.rs
(1 hunks)applications/tari_swarm_daemon/src/webserver/rpc/indexers.rs
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (18)
applications/tari_swarm_daemon/src/webserver/rpc/indexers.rs (2)
25-25
: LGTM! Thegraphql
field is correctly added to theIndexerInfo
struct.The field follows the same pattern as the existing
jrpc
field, maintaining consistency in the data structure.
36-36
: LGTM! The GraphQL URL is correctly retrieved and included in the response.The implementation follows the same pattern as the existing JSON-RPC URL handling.
Also applies to: 44-44
applications/tari_indexer_web_ui/src/utils/graphql.tsx (1)
23-27
: LGTM! The GraphQL URL handling is well-implemented.The implementation includes:
- Environment variable configuration with fallback
- Comprehensive error handling
- Default URL fallback
Also applies to: 29-45
applications/tari_swarm_daemon/src/process_definitions/indexer.rs (2)
26-26
: LGTM! The GraphQL port allocation is correctly implemented.The implementation follows the same pattern as the existing JSON-RPC and web UI ports.
Also applies to: 31-31, 33-33
58-58
: LGTM! The GraphQL command arguments are correctly added.The implementation follows the same pattern as the existing JSON-RPC arguments.
Also applies to: 61-61
applications/tari_indexer/src/http_ui/server.rs (1)
38-38
: LGTM! The GraphQL address endpoint is correctly implemented.The implementation:
- Correctly updates the function signature
- Follows the same pattern as the existing JSON-RPC endpoint
Also applies to: 43-43
applications/tari_swarm_daemon/src/process_definitions/context.rs (2)
96-99
: LGTM: Error message clarification in JSON RPC URL method.The error message has been updated to be more specific about the URL type.
101-117
: LGTM: Well-structured GraphQL URL implementation.The new
get_public_graphql_url
method follows the established pattern, with proper error handling and fallback to default IP.applications/tari_indexer/src/cli.rs (3)
59-61
: LGTM: Improved documentation clarity for JSON RPC URL.The documentation now correctly specifies this is for the JSON RPC address.
62-64
: LGTM: Well-documented GraphQL URL configuration.The new option is properly documented and includes environment variable support.
102-107
: LGTM: Consistent configuration override handling.The GraphQL URL override follows the same pattern as other configuration options.
applications/tari_indexer/src/config.rs (2)
86-88
: LGTM: Improved documentation for JSON RPC URL configuration.The documentation clearly explains the fallback behavior.
137-137
: LGTM: Proper default initialization.The new field is correctly initialized to
None
in the default implementation.applications/tari_indexer_web_ui/src/routes/Events/Events.tsx (2)
35-35
: LGTM: Clean utility function import.The GraphQL utility function is properly imported.
65-65
: LGTM: Dynamic GraphQL address integration.The fetch call now correctly uses the dynamically retrieved GraphQL address.
applications/tari_indexer/src/lib.rs (3)
140-144
: LGTM!The event manager initialization is clean and follows good practices by using Arc for shared ownership.
146-151
: LGTM!The GraphQL server startup is well-implemented with proper logging and error handling.
179-189
: LGTM!The public GraphQL URL handling follows the same pattern as the JSON-RPC URL handling, ensuring consistency in the codebase.
try { | ||
return new URL(url); | ||
} catch (e) { | ||
throw new Error(`Invalid URL: ${url} : {e}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the template literal syntax in error message.
The error message has incorrect template literal syntax for the error object.
- throw new Error(`Invalid URL: ${url} : {e}`);
+ throw new Error(`Invalid URL: ${url} : ${e}`);
📝 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.
throw new Error(`Invalid URL: ${url} : {e}`); | |
throw new Error(`Invalid URL: ${url} : ${e}`); |
pub fn get_public_graphql_url(&self) -> Url { | ||
match self.settings.get("public_graphql_url") { | ||
Some(url) => url.parse().expect("Invalid JSON RPC URL"), | ||
None => { | ||
let public_ip = self | ||
.settings | ||
.get("public_ip") | ||
.map(|s| s.as_str()) | ||
.unwrap_or("127.0.0.1"); | ||
let web_port = self.ports.get("jrpc").expect("jrpc port not found"); | ||
format!("http://{public_ip}:{web_port}/json_rpc") | ||
.parse() | ||
.expect("Invalid web URL") | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect URL construction in get_public_graphql_url
.
The method has two issues:
- It's incorrectly using the JSON-RPC port and path for the GraphQL URL.
- The error messages mention "JSON RPC URL" instead of "GraphQL URL".
Apply this diff to fix the issues:
pub fn get_public_graphql_url(&self) -> Url {
match self.settings.get("public_graphql_url") {
- Some(url) => url.parse().expect("Invalid JSON RPC URL"),
+ Some(url) => url.parse().expect("Invalid GraphQL URL"),
None => {
let public_ip = self
.settings
.get("public_ip")
.map(|s| s.as_str())
.unwrap_or("127.0.0.1");
- let web_port = self.ports.get("jrpc").expect("jrpc port not found");
- format!("http://{public_ip}:{web_port}/json_rpc")
+ let web_port = self.ports.get("web").expect("web port not found");
+ format!("http://{public_ip}:{web_port}/graphql")
.parse()
- .expect("Invalid web URL")
+ .expect("Invalid GraphQL URL")
},
}
}
📝 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.
pub fn get_public_graphql_url(&self) -> Url { | |
match self.settings.get("public_graphql_url") { | |
Some(url) => url.parse().expect("Invalid JSON RPC URL"), | |
None => { | |
let public_ip = self | |
.settings | |
.get("public_ip") | |
.map(|s| s.as_str()) | |
.unwrap_or("127.0.0.1"); | |
let web_port = self.ports.get("jrpc").expect("jrpc port not found"); | |
format!("http://{public_ip}:{web_port}/json_rpc") | |
.parse() | |
.expect("Invalid web URL") | |
}, | |
} | |
} | |
pub fn get_public_graphql_url(&self) -> Url { | |
match self.settings.get("public_graphql_url") { | |
Some(url) => url.parse().expect("Invalid GraphQL URL"), | |
None => { | |
let public_ip = self | |
.settings | |
.get("public_ip") | |
.map(|s| s.as_str()) | |
.unwrap_or("127.0.0.1"); | |
let web_port = self.ports.get("web").expect("web port not found"); | |
format!("http://{public_ip}:{web_port}/graphql") | |
.parse() | |
.expect("Invalid GraphQL URL") | |
}, | |
} | |
} |
if let (Some(jrpc_address), Some(graphql_address)) = (jrpc_address, graphql_address) { | ||
info!(target: LOG_TARGET, "🌐 Started JSON-RPC server on {}", jrpc_address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Reconsider the dependency between JSON-RPC and GraphQL servers.
The JSON-RPC server startup now requires the GraphQL address to be configured, which creates an unnecessary dependency between these services. They should be able to operate independently.
Apply this diff to decouple the services:
- if let (Some(jrpc_address), Some(graphql_address)) = (jrpc_address, graphql_address) {
+ if let Some(jrpc_address) = jrpc_address {
📝 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.
if let (Some(jrpc_address), Some(graphql_address)) = (jrpc_address, graphql_address) { | |
info!(target: LOG_TARGET, "🌐 Started JSON-RPC server on {}", jrpc_address); | |
if let Some(jrpc_address) = jrpc_address { | |
info!(target: LOG_TARGET, "🌐 Started JSON-RPC server on {}", jrpc_address); |
Test Results (CI)617 tests ±0 616 ✅ ±0 3h 28m 1s ⏱️ - 13m 28s For more details on these failures, see this check. Results for commit dabc8cf. ± Comparison against base commit 737599c. ♻️ This comment has been updated with latest results. |
86f378e
Description --- Fix indexer config that requires JSONRPC and GraphQL to both be configured for web to work. Correct GraphQL url Clear terminated instances from list Display settings in swarm UI Motivation and Context --- #1303 (review) How Has This Been Tested? --- Manually What process can a PR reviewer use to test or verify this change? --- Run swarm Breaking Changes --- - [x] None - [ ] Requires data directory to be deleted - [ ] Other - Please specify <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - The node details view now includes a clickable GraphQL endpoint link, enhancing accessibility. - Instance management has been improved with capabilities for removal and automatic cleanup of terminated instances. - **Bug Fixes** - Error messages have been refined to provide clearer guidance for configuration issues, reducing unexpected runtime interruptions. - Enhanced error handling for invalid URL formats in GraphQL address retrieval. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
/graphql_address
endpoint to get the actual GraphQL URL, in similar fashion as the JSON RPC URL.Motivation and Context
When deploying on ContractNet, the events page is empty. This is due to the GraphQL URL being hardcoded to localhost.
This PR fixes it by parameterizing the public address in a similar way to what we already do for the JSON RPC public address
How Has This Been Tested?
Locally spawning
tari_swarm
and using the indexer web uiWhat process can a PR reviewer use to test or verify this change?
See previous section
Breaking Changes
Summary by CodeRabbit
New Features
Documentation