Skip to content
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.

Sign up for GitHub

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(graphql_playground): use graphiql instead #2446

Merged
merged 4 commits into from
Nov 25, 2024
Merged

fix(graphql_playground): use graphiql instead #2446

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc4f659
fix(graphql_playground): use graphiql instead
rymnc Nov 21, 2024
8c4acd5
fix: changelog
rymnc Nov 21, 2024
3fb499a
chore: bump max recursive depth
rymnc Nov 21, 2024
8460412
fix: move to breaking changes
rymnc Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- [2369](https://github.com/FuelLabs/fuel-core/pull/2369): The `transaction_insertion_time_in_thread_pool_milliseconds` metric is properly collected.
- [2413](https://github.com/FuelLabs/fuel-core/issues/2413): block production immediately errors if unable to lock the mutex.
- [2389](https://github.com/FuelLabs/fuel-core/pull/2389): Fix construction of reverse iterator in RocksDB.
- [2446](https://github.com/FuelLabs/fuel-core/pull/2446): Use graphiql instead of graphql-playground due to known vulnerability and stale development.
netrome marked this conversation as resolved.
Show resolved Hide resolved

### Changed
- [2378](https://github.com/FuelLabs/fuel-core/pull/2378): Use cached hash of the topic instead of calculating it on each publishing gossip message.
Expand Down
49 changes: 49 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion bin/fuel-core/src/cli/run/graphql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub struct GraphQLArgs {
pub graphql_max_complexity: usize,

/// The max recursive depth of GraphQL queries.
#[clap(long = "graphql-max-recursive-depth", default_value = "16", env)]
#[clap(long = "graphql-max-recursive-depth", default_value = "24", env)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16 is not sufficient for graphiql, needing this bump

pub graphql_max_recursive_depth: usize,

/// The max resolver recursive depth of GraphQL queries.
Expand Down
2 changes: 1 addition & 1 deletion crates/fuel-core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ version = { workspace = true }
[dependencies]
anyhow = { workspace = true }
async-graphql = { version = "7.0.11", features = [
"playground",
"graphiql",
"tracing",
], default-features = false }
async-graphql-value = "7.0.11"
Expand Down
30 changes: 20 additions & 10 deletions crates/fuel-core/src/graphql_api/api_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@ use crate::{
},
};
use async_graphql::{
http::{
playground_source,
GraphQLPlaygroundConfig,
},
http::GraphiQLSource,
Request,
Response,
};
Expand Down Expand Up @@ -278,16 +275,22 @@ where
.extension(ViewExtension::new())
.finish();

let graphql_endpoint = "/v1/graphql";
let graphql_subscription_endpoint = "/v1/graphql-sub";

let graphql_playground =
|| render_graphql_playground(graphql_endpoint, graphql_subscription_endpoint);

let router = Router::new()
.route("/v1/playground", get(graphql_playground))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can support old endpoint under v1 and new under v2=)

Copy link
Contributor

@AurelienFT AurelienFT Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point ? I think the new one has all the same features and it's just a playground no code depends on it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case this change will be non breaking and we don't need to update documentation plus people who used to use the old playground can continue to use it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old playground has security issues too so I'm not sure we should keep it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me when you perform an update, you can except some UI changes as soon as it doesn't break any code I don't think anyone is doing scrapping on the graphql playground and I see no point of using the previous ones all features seems to exist on this one too.

Copy link
Contributor

@AurelienFT AurelienFT Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I don't want to block anything just "for this" so it's a deal breaker for you @xgreenx I'm ok to walk beside you on that.

.route(
"/v1/graphql",
graphql_endpoint,
post(graphql_handler)
.layer(ConcurrencyLimitLayer::new(concurrency_limit))
.options(ok),
)
.route(
"/v1/graphql-sub",
graphql_subscription_endpoint,
post(graphql_subscription_handler).options(ok),
)
.route("/v1/metrics", get(metrics))
Expand Down Expand Up @@ -325,10 +328,17 @@ where
))
}

async fn graphql_playground() -> impl IntoResponse {
Html(playground_source(GraphQLPlaygroundConfig::new(
"/v1/graphql",
)))
async fn render_graphql_playground(
endpoint: &str,
subscription_endpoint: &str,
) -> impl IntoResponse {
Html(
GraphiQLSource::build()
.endpoint(endpoint)
.subscription_endpoint(subscription_endpoint)
.title("Fuel Graphql Playground")
.finish(),
)
}

async fn health() -> Json<serde_json::Value> {
Expand Down
Loading