Fix serialisation bug when a fragment spread was not exhaustive

pkqk commented 1 year ago

When a response included multiple implementations of an interface type but the query only asked for certain types in the fragment spread invalid json could be generated, as a loop around the serialisation function added in a comma but without any accompanying content.

For this example schema

interface Gizmo {
    id: ID!
    name: String!
}

type Gadget implements Gizmo {
    id: ID!
    name: String!
    owner: String!
}

type Tool implements Gizmo {
    id: ID!
    name: String!
    category: String!
}

type Query {
    gizmos: [Gizmo!]!
}

The query

query Gizmo {
    gizmos {
        id
        ...GizmoDetails
    }
}

fragment GizmoDetails on Gizmo {
    ... on Gadget {
        name
        owner
    }
}

would end up with invalid JSON for the Tool Gizmo as it would write a , after the id field expecting to then go through the fragment and write more values.

This changes the formatResponseDataRec so that it collects the next value, checks it has content and only writes a , if it's about to write more data after that. Changing the general logic from write a comma after everything except the last item to, writing a comma before every item after the first one.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 :tada:

Comparison is base (eff66a8) 70.93% compared to head (259157a) 71.00%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #215 +/- ## ========================================== + Coverage 70.93% 71.00% +0.07% ========================================== Files 27 27 Lines 2735 2735 ========================================== + Hits 1940 1942 +2 + Misses 672 671 -1 + Partials 123 122 -1 ``` | [Impacted Files](https://app.codecov.io/gh/movio/bramble/pull/215?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=movio) | Coverage Δ | | |---|---|---| | [execution\_result.go](https://app.codecov.io/gh/movio/bramble/pull/215?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=movio#diff-ZXhlY3V0aW9uX3Jlc3VsdC5nbw==) | `71.80% <100.00%> (+0.88%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lucianjon commented 1 year ago

Makes sense! I do think using a structured builder would be a lot safer than string building as it would effectively guarantee well-formed json.

LGTM too, do agree on not manually constructing json strings but it's a refactoring we haven't had the time for yet.

movio / bramble

Fix serialisation bug when a fragment spread was not exhaustive #215

Codecov Report