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.

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

NON_NULL_TYPE node is missing BANG SyntaxKind #142

Closed
kpeters-cbsi opened this issue Jan 13, 2022 · 4 comments · Fixed by #146
Closed

NON_NULL_TYPE node is missing BANG SyntaxKind #142

kpeters-cbsi opened this issue Jan 13, 2022 · 4 comments · Fixed by #146
Assignees
Labels
bug Something isn't working

Comments

@kpeters-cbsi
Copy link

Description

This applies to apollo-parser v0.2.0

When a mutation is parsed into a Document doc, doc.toString() does not correctly preserve the "required" state of the variables.

mutation MyMutation($custId: Int!) {
    myMutation(custId: $custId)
}

Steps to reproduce

Execute the following Rust program:

use apollo_parser::Parser;

const MUTATION: &str = r#"mutation MyMutation($custId: Int!) {
    myMutation(custId: $custId)
}"#;


fn main() {
    let parser = Parser::new(MUTATION);
    let ast = parser.parse();
    if ast.errors().len() > 0 {
        dbg!(ast.errors());
        panic!("Parse error");
    } else {
        let doc = ast.document();
        println!("{}", doc.to_string());
    }
}

Expected result

Should produce a semantically identical copy of the response having $custId marked as required.

mutation MyMutation($custId: Int!) {
    myMutation(custId: $custId)
}

Actual result

Produced a copy of the response where $custId was not a required variable.

mutation MyMutation($custId: Int) {
    myMutation(custId: $custId)
}

Environment

  • Operating system and version: MacOS Big Sur 11.6
  • Shell (bash/zsh/powershell): zsh
  • Crate version: 0.2.0
@kpeters-cbsi kpeters-cbsi added bug Something isn't working triage labels Jan 13, 2022
@lrlna
Copy link
Member

lrlna commented Jan 20, 2022

@kpeters-cbsi thank you so much for submitting this! It seems like we are missing the BANG (!) token in the AST. The node gets recorded as as NON_NULL_TYPE, but it doesn't have the token that actually makes it NON_NULL. This is an easy fix, and I can fix and publish a patch for you tomorrow.

@lrlna lrlna added 2022-01 and removed triage labels Jan 20, 2022
lrlna added a commit that referenced this issue Jan 21, 2022
We are missing BANG token in the AST when a NON_NULL_TYPE gets created. Although the node created is indeed NON_NULL_TYPE, it's also important to keep the original set of tokens. The NON_NULL_TYPE after this commit looks like this:

```txt
- [email protected]
    - [email protected] "\n    "
    - [email protected]
        - [email protected]
            - [email protected] "Int"
    - [email protected] "!"
```

fixes #142
@lrlna lrlna changed the title apolllo_parser::ast::Document.to_string() doesn't preserve "required" flag on variables NON_NULL_TYPE node is missing BANG SyntaxKind Jan 21, 2022
@lrlna lrlna self-assigned this Jan 21, 2022
lrlna added a commit that referenced this issue Jan 21, 2022
We are missing BANG token in the AST when a NON_NULL_TYPE gets created. Although the node created is indeed NON_NULL_TYPE, it's also important to keep the original set of tokens. The NON_NULL_TYPE after this commit looks like this:

```txt
- [email protected]
    - [email protected] "\n    "
    - [email protected]
        - [email protected]
            - [email protected] "Int"
    - [email protected] "!"
```

fixes #142
@kpeters-cbsi
Copy link
Author

So when will we see a new release incorporating this fix?

@lrlna
Copy link
Member

lrlna commented Jan 26, 2022

today! I found another issue sort of related to this in #143 which you'll run into as soon as you have more than one variable, for example:

mutation MyMutation($custId: Int!, $b: String ) {
    myMutation(custId: $custId)
}

the to_string() version of the doc is entirely nonsensical:

 mutation MyMutation($custId: , Int!$b:  String) {
    myMutation(custId: $custId)
}

@lrlna
Copy link
Member

lrlna commented Jan 26, 2022

@kpeters-cbsi you can now install 0.2.1 with the latest fix. Let me know if you run into trouble!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants