Skip to content

Commit

Permalink
[GraphQL][RFC] Introduce UInt scalar
Browse files Browse the repository at this point in the history
## Description

Whilst working on #18337, I noticed that we were over-using the `Int`
scalar -- using it to represent values that could exceed 2^31 - 1 --
when the GraphQL spec states that `Int` must be a 32-bit signed
integer.

We made this decision at the time (a) because `async-graphql` allowed
converting `u64`s to `Int` and we were primarily concerned with the
fact that although JSON doesn't specify a precision for its numeric
types, JS (among other languages), assumes it is an IEEE
double-precision floating point number, so can only represent integral
values precisely below 2^53.

`cynic` (a Rust GraphQL client library) is (correctly) stricter,
however, and maps an `Int` to an `i32`, always. There may be other
similarly strict client libraries for other languages.

This PR introduces a new scalar, `UInt`, that maps to a JSON number
literal, just like `Int`, but allows us to ascribe our own meaning (in
this case, it will be an unsigned number, and it can be as large as
2^53).

This scalar has been used in many cases where we had previously used
`Int`: sequence numbers, counts of objects, checkpoints, transactions,
etc. While other uses continue to use `Int` (pagination limits,
service limits, values bounded by the number of validators).

In some cases, we have switched from `BigInt` to using this
scalar notably:

- the db cost estimate, which was previously a `BigInt` because we
  were unsure of its scale, but in hindsight, after benchmarking, it
  is unlikely that we would want to set a limit greater than 2^31 - 1.

- the number of checkpoints in an epoch, as the number of transactions
  in an epoch (a number that is guaranteed to be greater) is being
  represented using an `Int` at the moment (and soon a `UInt`).

This will be a breaking change, so will only go out with the new major
version. Hopefully, this change will be minimal as the format of this
scalar over the wire is the same as for `Int`, but it will require
existing clients to register a new scalar in most cases.

## Test plan

Existing tests:

```
sui-graphql-rpc$ cargo nextest run
sui-graphql-e2e-tests$ cargo nextest run --features pg_integration
```
  • Loading branch information
amnn committed Jul 7, 2024
1 parent 488a9b1 commit d455b68
Show file tree
Hide file tree
Showing 37 changed files with 362 additions and 239 deletions.
104 changes: 55 additions & 49 deletions crates/sui-graphql-rpc/schema/current_progress_schema.graphql

Large diffs are not rendered by default.

27 changes: 13 additions & 14 deletions crates/sui-graphql-rpc/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// SPDX-License-Identifier: Apache-2.0

use crate::functional_group::FunctionalGroup;
use crate::types::big_int::BigInt;
use async_graphql::*;
use fastcrypto_zkp::bn254::zk_login_api::ZkLoginEnv;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -67,23 +66,23 @@ pub struct Limits {
/// Maximum number of nodes in the requests.
pub max_query_nodes: u32,
/// Maximum number of output nodes allowed in the response.
pub max_output_nodes: u64,
pub max_output_nodes: u32,
/// Maximum size (in bytes) of a GraphQL request.
pub max_query_payload_size: u32,
/// Queries whose EXPLAIN cost are more than this will be logged. Given in the units used by the
/// database (where 1.0 is roughly the cost of a sequential page access).
pub max_db_query_cost: u64,
pub max_db_query_cost: u32,
/// Paginated queries will return this many elements if a page size is not provided.
pub default_page_size: u64,
pub default_page_size: u32,
/// Paginated queries can return at most this many elements.
pub max_page_size: u64,
pub max_page_size: u32,
/// Time (in milliseconds) to wait for a transaction to be executed and the results returned
/// from GraphQL. If the transaction takes longer than this time to execute, the request will
/// return a timeout error, but the transaction may continue executing.
pub mutation_timeout_ms: u64,
pub mutation_timeout_ms: u32,
/// Time (in milliseconds) to wait for a read request from the GraphQL service. Requests that
/// take longer than this time to return a result will return a timeout error.
pub request_timeout_ms: u64,
pub request_timeout_ms: u32,
/// Maximum amount of nesting among type arguments (type arguments nest when a type argument is
/// itself generic and has arguments).
pub max_type_argument_depth: u32,
Expand Down Expand Up @@ -223,36 +222,36 @@ impl ServiceConfig {
/// with a connection of first: 10 and has a field to a connection with last: 20, the count
/// at the second level would be 200 nodes. This is then summed to the count of 10 nodes
/// at the first level, for a total of 210 nodes.
pub async fn max_output_nodes(&self) -> u64 {
pub async fn max_output_nodes(&self) -> u32 {
self.limits.max_output_nodes
}

/// Maximum estimated cost of a database query used to serve a GraphQL request. This is
/// measured in the same units that the database uses in EXPLAIN queries.
async fn max_db_query_cost(&self) -> BigInt {
BigInt::from(self.limits.max_db_query_cost)
async fn max_db_query_cost(&self) -> u32 {
self.limits.max_db_query_cost
}

/// Default number of elements allowed on a single page of a connection.
async fn default_page_size(&self) -> u64 {
async fn default_page_size(&self) -> u32 {
self.limits.default_page_size
}

/// Maximum number of elements allowed on a single page of a connection.
async fn max_page_size(&self) -> u64 {
async fn max_page_size(&self) -> u32 {
self.limits.max_page_size
}

/// Maximum time in milliseconds spent waiting for a response from fullnode after issuing a
/// a transaction to execute. Note that the transaction may still succeed even in the case of a
/// timeout. Transactions are idempotent, so a transaction that times out should be resubmitted
/// until the network returns a definite response (success or failure, not timeout).
async fn mutation_timeout_ms(&self) -> u64 {
async fn mutation_timeout_ms(&self) -> u32 {
self.limits.mutation_timeout_ms
}

/// Maximum time in milliseconds that will be spent to serve one query request.
async fn request_timeout_ms(&self) -> u64 {
async fn request_timeout_ms(&self) -> u32 {
self.limits.request_timeout_ms
}

Expand Down
4 changes: 2 additions & 2 deletions crates/sui-graphql-rpc/src/data/pg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub(crate) struct PgExecutor {
}

pub(crate) struct PgConnection<'c> {
max_cost: u64,
max_cost: u32,
conn: &'c mut diesel::PgConnection,
}

Expand Down Expand Up @@ -147,7 +147,7 @@ mod query_cost {
}

/// Run `EXPLAIN` on the `query`, and log the estimated cost.
pub(crate) fn log<Q>(conn: &mut PgConnection, max_db_query_cost: u64, query: Q)
pub(crate) fn log<Q>(conn: &mut PgConnection, max_db_query_cost: u32, query: Q)
where
Q: Query + QueryId + QueryFragment<Pg> + RunQueryDsl<PgConnection>,
{
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-graphql-rpc/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub enum Error {
#[error("'first' and 'last' must not be used together")]
CursorNoFirstLast,
#[error("Connection's page size of {0} exceeds max of {1}")]
PageTooLarge(u64, u64),
PageTooLarge(u64, u32),
// Catch-all for client-fault errors
#[error("{0}")]
Client(String),
Expand Down
16 changes: 8 additions & 8 deletions crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub(crate) struct ShowUsage;
#[derive(Clone, Debug, Default)]
struct ValidationRes {
input_nodes: u32,
output_nodes: u64,
output_nodes: u32,
depth: u32,
num_variables: u32,
num_fragments: u32,
Expand Down Expand Up @@ -73,7 +73,7 @@ impl ExtensionFactory for QueryLimitsChecker {
#[derive(Debug)]
struct ComponentCost {
pub input_nodes: u32,
pub output_nodes: u64,
pub output_nodes: u32,
pub depth: u32,
}

Expand Down Expand Up @@ -234,7 +234,7 @@ impl QueryLimitsChecker {
// Use BFS to analyze the query and count the number of nodes and the depth of the query
struct ToVisit<'s> {
selection: &'s Positioned<Selection>,
parent_node_count: u64,
parent_node_count: u32,
}

// Queue to store the nodes at each level
Expand Down Expand Up @@ -431,8 +431,8 @@ fn check_directives(directives: &[Positioned<Directive>]) -> ServerResult<()> {
fn estimate_output_nodes_for_curr_node(
f: &Positioned<Field>,
variables: &Variables,
default_page_size: u64,
) -> u64 {
default_page_size: u32,
) -> u32 {
if !is_connection(f) {
1
} else {
Expand All @@ -447,18 +447,18 @@ fn estimate_output_nodes_for_curr_node(
}

/// Try to extract a u64 value from the given argument, or return None on failure.
fn extract_limit(value: Option<&Positioned<GqlValue>>, variables: &Variables) -> Option<u64> {
fn extract_limit(value: Option<&Positioned<GqlValue>>, variables: &Variables) -> Option<u32> {
if let GqlValue::Variable(var) = &value?.node {
return match variables.get(var) {
Some(Value::Number(num)) => num.as_u64(),
Some(Value::Number(num)) => num.as_u64().map(|v| v as u32),
_ => None,
};
}

let GqlValue::Number(value) = &value?.node else {
return None;
};
value.as_u64()
value.as_u64().map(|v| v as u32)
}

/// Checks if the given field is a connection field by whether it has 'edges' or 'nodes' selected.
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-graphql-rpc/src/extensions/timeout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ impl Extension for TimeoutExt {
// increase the timeout if the request is a mutation
let is_mutation = self.is_mutation.load(Ordering::Relaxed);
let request_timeout = if is_mutation {
Duration::from_millis(cfg.limits.mutation_timeout_ms)
Duration::from_millis(cfg.limits.mutation_timeout_ms.into())
} else {
Duration::from_millis(cfg.limits.request_timeout_ms)
Duration::from_millis(cfg.limits.request_timeout_ms.into())
};

timeout(request_timeout, next.run(ctx, operation_name))
Expand Down
8 changes: 4 additions & 4 deletions crates/sui-graphql-rpc/src/server/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ impl ServerBuilder {
// Bound each statement in a request with the overall request timeout, to bound DB
// utilisation (in the worst case we will use 2x the request timeout time in DB wall
// time).
config.service.limits.request_timeout_ms,
config.service.limits.request_timeout_ms.into(),
)
.map_err(|e| Error::Internal(format!("Failed to create pg connection pool: {}", e)))?;

Expand Down Expand Up @@ -686,7 +686,7 @@ pub mod tests {
let reader = PgManager::reader_with_config(
connection_config.db_url.clone(),
connection_config.db_pool_size,
service_config.limits.request_timeout_ms,
service_config.limits.request_timeout_ms.into(),
)
.expect("Failed to create pg connection pool");

Expand Down Expand Up @@ -769,8 +769,8 @@ pub mod tests {
sui_client: &SuiClient,
) -> Response {
let mut cfg = ServiceConfig::default();
cfg.limits.request_timeout_ms = timeout.as_millis() as u64;
cfg.limits.mutation_timeout_ms = timeout.as_millis() as u64;
cfg.limits.request_timeout_ms = timeout.as_millis() as u32;
cfg.limits.mutation_timeout_ms = timeout.as_millis() as u32;

let schema = prep_schema(None, Some(cfg))
.context_data(Some(sui_client.clone()))
Expand Down
5 changes: 3 additions & 2 deletions crates/sui-graphql-rpc/src/types/balance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use super::available_range::AvailableRange;
use super::cursor::{self, Page, RawPaginated, Target};
use super::uint::UInt;
use super::{big_int::BigInt, move_type::MoveType, sui_address::SuiAddress};
use crate::consistency::Checkpointed;
use crate::data::{Db, DbConnection, QueryExecutor};
Expand All @@ -26,7 +27,7 @@ pub(crate) struct Balance {
/// Coin type for the balance, such as 0x2::sui::SUI
pub(crate) coin_type: MoveType,
/// How many coins of this type constitute the balance
pub(crate) coin_object_count: Option<u64>,
pub(crate) coin_object_count: Option<UInt>,
/// Total balance across all coin objects of the coin type
pub(crate) total_balance: Option<BigInt>,
}
Expand Down Expand Up @@ -174,7 +175,7 @@ impl TryFrom<StoredBalance> for Balance {
.transpose()
.map_err(|_| Error::Internal("Failed to read balance.".to_string()))?;

let coin_object_count = count.map(|c| c as u64);
let coin_object_count = count.map(|c| UInt::from(c as u64));

let coin_type = MoveType::new(
parse_sui_type_tag(&coin_type)
Expand Down
17 changes: 9 additions & 8 deletions crates/sui-graphql-rpc/src/types/checkpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use super::{
epoch::Epoch,
gas::GasCostSummary,
transaction_block::{self, TransactionBlock, TransactionBlockFilter},
uint::UInt,
};
use crate::consistency::Checkpointed;
use crate::{
Expand All @@ -32,7 +33,7 @@ use sui_types::messages_checkpoint::CheckpointDigest;
#[derive(Default, InputObject)]
pub(crate) struct CheckpointId {
pub digest: Option<Digest>,
pub sequence_number: Option<u64>,
pub sequence_number: Option<UInt>,
}

/// DataLoader key for fetching a `Checkpoint` by its sequence number, constrained by a consistency
Expand Down Expand Up @@ -90,8 +91,8 @@ impl Checkpoint {

/// This checkpoint's position in the total order of finalized checkpoints, agreed upon by
/// consensus.
async fn sequence_number(&self) -> u64 {
self.sequence_number_impl()
async fn sequence_number(&self) -> UInt {
self.sequence_number_impl().into()
}

/// The timestamp at which the checkpoint is agreed to have happened according to consensus.
Expand All @@ -115,8 +116,8 @@ impl Checkpoint {
}

/// The total number of transaction blocks in the network by the end of this checkpoint.
async fn network_total_transactions(&self) -> Option<u64> {
Some(self.network_total_transactions_impl())
async fn network_total_transactions(&self) -> Option<UInt> {
Some(self.network_total_transactions_impl().into())
}

/// The computation cost, storage cost, storage rebate, and non-refundable storage fee
Expand Down Expand Up @@ -157,7 +158,7 @@ impl Checkpoint {
let Some(filter) = filter
.unwrap_or_default()
.intersect(TransactionBlockFilter {
at_checkpoint: Some(self.stored.sequence_number as u64),
at_checkpoint: Some(UInt::from(self.stored.sequence_number as u64)),
..Default::default()
})
else {
Expand All @@ -178,7 +179,7 @@ impl Checkpoint {
impl CheckpointId {
pub(crate) fn by_seq_num(seq_num: u64) -> Self {
CheckpointId {
sequence_number: Some(seq_num),
sequence_number: Some(seq_num.into()),
digest: None,
}
}
Expand Down Expand Up @@ -213,7 +214,7 @@ impl Checkpoint {
} => {
let DataLoader(dl) = ctx.data_unchecked();
dl.load_one(SeqNumKey {
sequence_number,
sequence_number: sequence_number.into(),
digest,
checkpoint_viewed_at,
})
Expand Down
3 changes: 2 additions & 1 deletion crates/sui-graphql-rpc/src/types/coin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use super::sui_address::SuiAddress;
use super::suins_registration::{DomainFormat, SuinsRegistration};
use super::transaction_block::{self, TransactionBlock, TransactionBlockFilter};
use super::type_filter::ExactTypeFilter;
use super::uint::UInt;
use async_graphql::*;

use async_graphql::connection::{Connection, CursorType, Edge};
Expand Down Expand Up @@ -150,7 +151,7 @@ impl Coin {
.await
}

pub(crate) async fn version(&self) -> u64 {
pub(crate) async fn version(&self) -> UInt {
ObjectImpl(&self.super_.super_).version().await
}

Expand Down
3 changes: 2 additions & 1 deletion crates/sui-graphql-rpc/src/types/coin_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use super::sui_address::SuiAddress;
use super::suins_registration::{DomainFormat, SuinsRegistration};
use super::transaction_block::{self, TransactionBlock, TransactionBlockFilter};
use super::type_filter::ExactTypeFilter;
use super::uint::UInt;
use crate::data::Db;
use crate::error::Error;
use async_graphql::connection::Connection;
Expand Down Expand Up @@ -139,7 +140,7 @@ impl CoinMetadata {
.await
}

pub(crate) async fn version(&self) -> u64 {
pub(crate) async fn version(&self) -> UInt {
ObjectImpl(&self.super_.super_).version().await
}

Expand Down
6 changes: 3 additions & 3 deletions crates/sui-graphql-rpc/src/types/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ impl<C> Page<C> {
(limit, after, None, before) => Page {
after,
before,
limit: limit.unwrap_or(limits.default_page_size),
limit: limit.unwrap_or(limits.default_page_size as u64),
end: End::Front,
},

Expand All @@ -152,7 +152,7 @@ impl<C> Page<C> {
},
};

if page.limit > limits.max_page_size {
if page.limit > limits.max_page_size as u64 {
return Err(Error::PageTooLarge(page.limit, limits.max_page_size).extend());
}

Expand Down Expand Up @@ -797,7 +797,7 @@ mod tests {
#[test]
fn test_err_page_too_big() {
let config = ServiceConfig::default();
let too_big = config.limits.max_page_size + 1;
let too_big = config.limits.max_page_size as u64 + 1;
let err = Page::<JsonCursor<u64>>::from_params(&config, Some(too_big), None, None, None)
.unwrap_err();

Expand Down
Loading

0 comments on commit d455b68

Please sign in to comment.