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

Response construction #44

Closed
Geal opened this issue Oct 15, 2021 · 4 comments · Fixed by #86
Closed

Response construction #44

Geal opened this issue Oct 15, 2021 · 4 comments · Fixed by #86
Assignees

Comments

@Geal
Copy link
Contributor

Geal commented Oct 15, 2021

Is your feature request related to a problem? Please describe.
The response generated from subgraph responses does not match the query.
Examples with the federation demo:

field order

query ExampleQuery { me  {
  identifiant: id
  reviews { body }
 id 
} }

returns:

"data": {
    "moi": {
        "identifiant": "1",
        "__typename": "User",
        "id": "1",
        "reviews": [{
            "body": "Love it!"
        }, {
            "body": "Too expensive."
        }]
    }
}
}

The id field should appear after the reviews field, but because a first query to the accounts subgraph must be performed to obtained the id key, before requesting the review, the id is added to the response before the reviews

unnecessary data

query ExampleQuery {
 me  {
   reviews { body }
} }

returns

{
    "data": {
        "me": {
            "__typename": "User",
            "id": "1",
            "reviews": [{
                "body": "Love it!"
            }, {
                "body": "Too expensive."
            }]
        }
    }
}

The id field should not be there, but since it was used for the join, it was added by the response from the accounts subgraph.

These inconsistencies in the responses will block the work on integration testing #47, because they will generate a lot of differences between the gateway and router responses.

Describe the solution you'd like
The response should be created using the query plan, with only the required fields, in order, then subgraph responses would only be merged where it is necessary.

Describe alternatives you've considered
This work could be done as part of the future Rust query planner, but it looks like it can be done independently, only using the common query plan format.

@martijnwalraven
Copy link

martijnwalraven commented Oct 15, 2021

Note that the expected field ordering is not static however, but depends on the runtime types of objects and on the values of variables that are used in @skip or @include.

That means you need to follow the GraphQL execution algorithm at least in spirit, although there are simplifications you can make. This is the step that is most relevant to this issue: http://spec.graphql.org/draft/#sec-Field-Collection

The way execution is implemented in graphql-js (and most other GraphQL servers) is fairly inefficient, because it is all AST-based. At least some parts of operations can be analyzed statically, and you could even come up with a more efficient representation that is specifically targeted at response shape computation. I have some ideas, and this is also related to the testing work I'm doing, but there are benefits to supporting execution proper in Rust so that may not be worth adopting.

@Geal
Copy link
Contributor Author

Geal commented Oct 18, 2021

right, if it requires some runtime info too it can become very tricky. Also, apparently the query plan we currently get from the harmonizer does not give info on the expected fields.
Example: the query query ExampleQuery {\n me {\n identifiant:id \n reviews { body }\n }\n \n}
gives us this plan:

Sequence {                                                                                                       
    nodes: [                                                                                                           
        Fetch(                                                                                                         
            FetchNode {                                                                                                
                service_name: "accounts",                                                                              
                requires: None,                                                                                        
                variable_usages: [],                                                                                   
                operation: "{me{identifiant:id __typename id}}",                                                       
            },                                                                                                         
        ),                                                                                                             
        Flatten(                                                                                                       
            FlattenNode {                                  
                path: Path(                                                                                            
                    [                                                                                                  
                        Key(                                                                                           
                            "me",                          
                        ),                                                                                             
                    ],                                                                                                 
                ),
                node: Fetch(                                                                                           
                    FetchNode {                                                                                        
                        service_name: "reviews",                                                                       
                        requires: Some(                                                                                                 
                            [                                                                                                           
                                InlineFragment(                     
                                    InlineFragment {                                                                                    
                                        type_condition: Some(                                                                           
                                            "User",                                                                                     
                                        ),                                                                                              
                                        selections: [                                                                                   
                                            Field(                                                                                      
                                                Field {                                                                                 
                                                    alias: None,                                                                        
                                                    name: "__typename",                                                                 
                                                    selections: None,                                                                   
                                                },                                                                                      
                                            ),                                                                                          
                                            Field(                                                                                      
                                                Field {                                                                                 
                                                    alias: None,                                                                        
                                                    name: "id",                                                                         
                                                    selections: None,                                                                   
                                                },                  
                                            ),                      
                                        ],                          
                                    },                              
                                ),                                  
                            ],                                      
                        ),                                          
                        variable_usages: [],                        
                        operation: "query($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{body}}}}",
                    },                                              
                ),                                                  
            },                                                      
        ),                                                          
    ],                                                              
}

@cecton
Copy link
Contributor

cecton commented Oct 18, 2021

In the current state I don't think we can address this issue. A good way to solve it would be to improve the query planner to include the information that is missing. When we do re-write the query planner in Rust we will be able to create our own format and improve a few things here and there like this one.

@martijnwalraven
Copy link

martijnwalraven commented Oct 18, 2021

right, if it requires some runtime info too it can become very tricky. Also, apparently the query plan we currently get from the harmonizer does not give info on the expected fields.

Yeah, the query plan does not include the input query because that seems redundant. We could return a parsed representation of the query from JS in addition to the query plan if we wanted to avoid parsing in Rust, but it seems with apollo-rs we'd already have a similar representation in Rust.

@o0Ignition0o o0Ignition0o transferred this issue from another repository Nov 4, 2021
@cecton cecton closed this as completed in #86 Nov 5, 2021
cecton added a commit that referenced this issue Nov 5, 2021
@Geal Geal removed the triage label Jan 7, 2022
abernix pushed a commit that referenced this issue Mar 15, 2023
Fix query plan cache config

cache->experimental_cache
in_memory and experimental_cache are now defaulted

Final layout:

```yaml
supergraph:
  query_planning:
    experimental_cache:
      in_memory:
        limit: 2000
      redis:
        urls:
          - http://example.com/
```

---------

Co-authored-by: bryn <bryn@apollographql.com>
tinnou pushed a commit to Netflix-Skunkworks/router that referenced this issue Oct 16, 2023
Fixes apollographql#44

deno_core advocates for us to use snapshots instead of loading scripts and running them.
One of the recent side effects is that deno_core prints `rerun-if-changed` directives when loading scripts manually.

This commit moves the scripts loading mechanism to the build (in `build_harmonizer.rs`) so the messages mentioned above won't display at runtime.
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 a pull request may close this issue.

5 participants