movio / bramble

A federated GraphQL API gateway
https://movio.github.io/bramble/
MIT License
497 stars 55 forks source link

Use non recursive algorithm to tidy up SelectionSet for response formatting #155

Closed lucianjon closed 2 years ago

lucianjon commented 2 years ago

Background

When formatting the final response to be sent back to the user we need two pieces of data to help shape the JSON, this is: the merged downstream response data, and the request selection set. In the past we used the merged downstream response data as our guide to creating the final JSON response. However, this approach had lots of incompatibilities with fragments, especially fragments on abstract types. There is just not enough information in the merged response data to decide which fields should be non-null and which shouldn't when it comes to fragments.

We now instead use the request selection set to guide how the final JSON should look, which when combined with injected __typename data in the response, has enough information to correctly format the JSON. While the selection set generally follows the same shape as a JSON response, fragments again throw a wrench in here for two reasons:

Multiple spreads on abstract types

Consider this query:

query {
    snapshot(id: "GIZMO1") {
        id
        ... NamedFragment
    }
}

fragment NamedFragment on Snapshot {
    name
    ... on GizmoImplementation {
        gizmos {
            id
            name
        }
    }
    ... on GadgetImplementation {
        gadgets {
            id
            name
        }
    }
}

Now Snapshot is an abstract type and the spreads are both on implementation types. However only one of these will be matched in the downstream service and we'll get something like this as the response:

{
    "data": {
        "snapshot": {
            "id": "100",
            "name": "foo",
            "gadgets": [{ "_bramble_id": "GADGET1", "id": "GADGET1" }],
            "_bramble__typename": "GadgetImplementation"
        }
    }
}

The problem here is if we're using the selection set to infer the response JSON, we have this entire fragment with types that may be non-nullable but is rendered unnecessary by the fact the other fragment matched the underlying type. So we must use _bramble__typename to eliminate the unnecessary fragment.

Fields in fragments are bought up a level when represented in JSON

Consider this query:

query Foo {
    snapshot(id: "GIZMO1") {
        id
        name
        ... on GizmoImplementation {
            gizmos {
                id
                name
            }
        }
    }
}

The resulting JSON should look like:

{
    "snapshot": {
        "id": "100",
        "name": "foo",
        "gizmos": [{ "id": "GIZMO1", "name": "Gizmo #1" }]
    }
}

So this is the second issue, if we encounter a fragment, we must merge the top level fields with the fields of the fragment's outer scope.

Problem

We had a function to do both of these things unionAndTrimSelectionSet. The problem was it was recursive when it shouldn't have been. The place this function is called from is already recursively walking down the merged response data. This algorithm is only interested in the top level of the scope it's called in, as this is the only scope where the passed in _bramble__typename is accurate. If unionAndTrimSelectionSet walks too far down the tree it will be using an incorrect _bramble__typename when deciding what to eliminate.

Solution

Essentially remove the recursive element of this and split the logic in two: the first part will eliminate fragments that are not needed, the second will merge top level fragment fields with the current scope. Anything further than one level down can be left as is because as the outer function recurses down the tree it will call unionAndTrimSelectionSet again with the correct parameters for that given scope.

codecov-commenter commented 2 years ago

Codecov Report

Merging #155 (c9e02c6) into main (a33268f) will increase coverage by 0.35%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   70.31%   70.67%   +0.35%     
==========================================
  Files          25       25              
  Lines        2638     2670      +32     
==========================================
+ Hits         1855     1887      +32     
  Misses        663      663              
  Partials      120      120              
Impacted Files Coverage Δ
query_execution.go 78.79% <100.00%> (+1.40%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a33268f...c9e02c6. Read the comment docs.