Skip to content

fix(graphql): prevent value fields from shadowing response input - #299

Open
amnn wants to merge 1 commit into
masterfrom
amnn/gql-macro-shadow
Open

fix(graphql): prevent value fields from shadowing response input#299
amnn wants to merge 1 commit into
masterfrom
amnn/gql-macro-shadow

Conversation

@amnn

@amnn amnn commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Description

The Response derive generated one local binding per response field. When a field named value came before another field, its binding shadowed the value parameter containing the original JSON response, so later fields tried to traverse the extracted field instead of the response root, which often led to compilation (type) errors.

Before:

pub fn from_value(value: serde_json::Value) -> Result<Self, String> {
    let value = {
        let current = &value;
        // extract value
    };
    let digest = {
        let current = &value;
        // incorrectly extracts from the value field
    };
    Ok(Self { value, digest })
}

After:

pub fn from_value(value: serde_json::Value) -> Result<Self, String> {
    Ok(Self {
        value: {
            let current = &value;
            // extract value
        },
        digest: {
            let current = &value;
            // extracts from the original response
        },
    })
}

This change initializes fields directly in the struct literal so field names do not introduce local bindings.

Test Plan

Adds a regression test with a value field followed by another extraction.

$ cargo nextest run -p sui-graphql-macros --test extraction_test \
  -- test_field_named_value_does_not_shadow_response_value

The `Response` derive generated one local binding per response field.
When a field named `value` came before another field, its binding
shadowed the `value` parameter containing the original JSON response, so
later fields tried to traverse the extracted field instead of the
response root, which often led to compilation (type) errors.

Before:

```rust
pub fn from_value(value: serde_json::Value) -> Result<Self, String> {
    let value = {
        let current = &value;
        // extract value
    };
    let digest = {
        let current = &value;
        // incorrectly extracts from the value field
    };
    Ok(Self { value, digest })
}
```

After:

```rust
pub fn from_value(value: serde_json::Value) -> Result<Self, String> {
    Ok(Self {
        value: {
            let current = &value;
            // extract value
        },
        digest: {
            let current = &value;
            // extracts from the original response
        },
    })
}
```

This change initializes fields directly in the struct literal so field
names do not introduce local bindings.

## Test Plan

Adds a regression test with a `value` field followed by another
extraction.

```
$ cargo nextest run -p sui-graphql-macros --test extraction_test \
  -- test_field_named_value_does_not_shadow_response_value
```
@amnn
amnn requested a review from bmwill as a code owner August 14, 2026 12:40
@amnn amnn self-assigned this Aug 14, 2026
@amnn
amnn requested a review from tpham-mysten August 14, 2026 12:40

@tpham-mysten tpham-mysten left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great catch! Thanks @amnn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants