Skip to content

Commit

Permalink
[pick MystenLabs#18347][GraphQL] Fix bug where config types "zero out…
Browse files Browse the repository at this point in the history
…" fields not in TOML (MystenLabs#18380)

## Description

When a field is labeled with `#[serde(default)]` it means that if that
field is not present, it is populated using the `Default` impl for the
field's type:

```rust
#[derive(Serialize, Deserialize, Clone, Debug)]
struct Foo {
    #[serde(default)]
    bar: Bar,
}

Foo { bar: Bar::default() }
```

This is not the behaviour we want, however. We want fields that have not
been supplied to be populated with their value from the `Default` impl
of the type they are part of:

```rust
Foo { bar: Foo::default().bar }
```

Implementing this manually requires a lot of boilerplate -- each field
needs to have an associated function that returns its default value:

```rust
#[derive(Serialize, Deserialize, Clone, Debug)]
struct Foo {
    #[serde(default = "Foo::__default_bar")]
    bar: Bar,
}
```

So this PR introduces an attribute proc macro to perform this
transformation:

```rust
#[GraphQLConfig]
struct Foo {
    bar: Bar,
}
```

It also performs some other related clean-ups:

- Moves default values inline into the respective `Default` impls, to
prevent them being used in isolation.
- Move the documentation for what the field is onto the struct
definition, and the documentation for how the default was chosen onto
the `Default` impl.
- Moved the implementation of `Display` for `Version`.
- Got rid of `ConnectionConfig::ci_integration_test_cfg` as it is the
same as its `Default` impl now.
- Improved various docs about what various configs are.

## Test plan

Added a test for reading a `ServiceConfig` from an incomplete/partial
TOML:

```sh
sui-graphql-rpc$ cargo nextest run -- read_partial_in_service_config
```

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [x] GraphQL: Fix a bug where starting the service using a config with
not all fields set would result in the unset fields being zeroed out
rather than taking their usual default values (as would happen if no
config had been explicitly supplied).
- [ ] CLI:
- [ ] Rust SDK:
  • Loading branch information
amnn committed Jun 26, 2024
1 parent c91a172 commit a5b8e86
Show file tree
Hide file tree
Showing 12 changed files with 323 additions and 191 deletions.
9 changes: 9 additions & 0 deletions Cargo.lock

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

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ members = [
"crates/sui-framework-snapshot",
"crates/sui-framework-tests",
"crates/sui-genesis-builder",
"crates/sui-graphql-config",
"crates/sui-graphql-e2e-tests",
"crates/sui-graphql-rpc",
"crates/sui-graphql-rpc-client",
Expand Down Expand Up @@ -600,6 +601,7 @@ sui-faucet = { path = "crates/sui-faucet" }
sui-framework = { path = "crates/sui-framework" }
sui-framework-snapshot = { path = "crates/sui-framework-snapshot" }
sui-framework-tests = { path = "crates/sui-framework-tests" }
sui-graphql-config = { path = "crates/sui-graphql-config" }
sui-graphql-rpc = { path = "crates/sui-graphql-rpc" }
sui-graphql-rpc-client = { path = "crates/sui-graphql-rpc-client" }
sui-graphql-rpc-headers = { path = "crates/sui-graphql-rpc-headers" }
Expand Down
14 changes: 14 additions & 0 deletions crates/sui-graphql-config/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[package]
name = "sui-graphql-config"
version.workspace = true
authors = ["Mysten Labs <build@mystenlabs.com"]
license = "Apache-2.0"
publish = false
edition = "2021"

[lib]
proc-macro = true

[dependencies]
quote.workspace = true
syn.workspace = true
143 changes: 143 additions & 0 deletions crates/sui-graphql-config/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use std::collections::BTreeSet;

use proc_macro::TokenStream;
use quote::{format_ident, quote};
use syn::{
parse_macro_input, Attribute, Data, DataStruct, DeriveInput, Fields, FieldsNamed, Ident, Meta,
NestedMeta,
};

/// Attribute macro to be applied to config-based structs. It ensures that the struct derives serde
/// traits, and `Debug`, that all fields are renamed with "kebab case", and adds a `#[serde(default
/// = ...)]` implementation for each field that ensures that if the field is not present during
/// deserialization, it is replaced with its default value, from the `Default` implementation for
/// the config struct.
#[allow(non_snake_case)]
#[proc_macro_attribute]
pub fn GraphQLConfig(_attr: TokenStream, input: TokenStream) -> TokenStream {
let DeriveInput {
attrs,
vis,
ident,
generics,
data,
} = parse_macro_input!(input as DeriveInput);

let Data::Struct(DataStruct {
struct_token,
fields,
semi_token,
}) = data
else {
panic!("GraphQL configs must be structs.");
};

let Fields::Named(FieldsNamed {
brace_token: _,
named,
}) = fields
else {
panic!("GraphQL configs must have named fields.");
};

// Figure out which derives need to be added to meet the criteria of a config struct.
let core_derives = core_derives(&attrs);

// Extract field names once to avoid having to check for their existence multiple times.
let fields_with_names: Vec<_> = named
.iter()
.map(|field| {
let Some(ident) = &field.ident else {
panic!("All fields must have an identifier.");
};

(ident, field)
})
.collect();

// Generate the fields with the `#[serde(default = ...)]` attribute.
let fields = fields_with_names.iter().map(|(name, field)| {
let default = format!("{ident}::__default_{name}");
quote! { #[serde(default = #default)] #field }
});

// Generate the default implementations for each field.
let defaults = fields_with_names.iter().map(|(name, field)| {
let ty = &field.ty;
let fn_name = format_ident!("__default_{}", name);
let cfg = extract_cfg(&field.attrs);

quote! {
#[doc(hidden)] #cfg
fn #fn_name() -> #ty {
Self::default().#name
}
}
});

TokenStream::from(quote! {
#[derive(#(#core_derives),*)]
#[serde(rename_all = "kebab-case")]
#(#attrs)* #vis #struct_token #ident #generics {
#(#fields),*
} #semi_token

impl #ident {
#(#defaults)*
}
})
}

/// Return a set of derives that should be added to the struct to make sure it derives all the
/// things we expect from a config, namely `Serialize`, `Deserialize`, and `Debug`.
///
/// We cannot add core derives unconditionally, because they will conflict with existing ones.
fn core_derives(attrs: &[Attribute]) -> BTreeSet<Ident> {
let mut derives = BTreeSet::from_iter([
format_ident!("Serialize"),
format_ident!("Deserialize"),
format_ident!("Debug"),
format_ident!("Clone"),
format_ident!("Eq"),
format_ident!("PartialEq"),
]);

for attr in attrs {
let Ok(Meta::List(list)) = attr.parse_meta() else {
continue;
};

let Some(ident) = list.path.get_ident() else {
continue;
};

if ident != "derive" {
continue;
}

for nested in list.nested {
let NestedMeta::Meta(Meta::Path(path)) = nested else {
continue;
};

let Some(ident) = path.get_ident() else {
continue;
};

derives.remove(ident);
}
}

derives
}

/// Find the attribute that corresponds to a `#[cfg(...)]` annotation, if it exists.
fn extract_cfg(attrs: &[Attribute]) -> Option<&Attribute> {
attrs.iter().find(|attr| {
let meta = attr.parse_meta().ok();
meta.is_some_and(|m| m.path().is_ident("cfg"))
})
}
1 change: 1 addition & 0 deletions crates/sui-graphql-rpc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ uuid.workspace = true
im.workspace = true
downcast = "0.11.0"

sui-graphql-config.workspace = true
sui-graphql-rpc-headers.workspace = true
sui-graphql-rpc-client.workspace = true

Expand Down
Loading

0 comments on commit a5b8e86

Please sign in to comment.