Skip to content
This repository has been archived by the owner on Oct 25, 2024. It is now read-only.

enhancement: refactor GraphQL schema parser #1281

Merged
merged 18 commits into from
Aug 29, 2023

Conversation

lostman
Copy link
Contributor

@lostman lostman commented Aug 18, 2023

Description

This PR refactors the ParsedGraphQLSchema::new() function and removes unnecessary content from the struct.

All parsing logic is moved to a separate struct, SchemaDecoder.

ServiceDocument and raw GraphQLSchema are removed from ParsedGraphQLSchema. ServiceDocument was only used in the code generator, and we only used the type definitions. We don't need both since we already have these in the parsed content—in type_defs. As for GraphQLSchema, we only need the version, not the raw schema String.

Testing steps

CI testing is sufficient.

Changelog

  • Move all parsing code out of ParsedGraphQLSchema::new() and into a separate struct.
  • Remove ast: ServiceDocument
  • Remove schema: GraphQLSchema
  • Add version: String (the version from GraphQLSchema)

Please add neat Changelog info here, according to our Contributor's Guide.

@lostman lostman force-pushed the maciej/1157-refactor-graphql-schema branch from 79d1868 to 924f565 Compare August 18, 2023 10:58
@ra0x3
Copy link
Contributor

ra0x3 commented Aug 18, 2023

@lostman

  • I haven't looked at any of the file changes since you haven't pinged this for review, but just a couple of notes/points
  • We will most likely be caching this ParsedGraphQLSchema (inside of IndexerSchema) when web requests come in, so we have a vested interest in not cloning TypeDefinitions if we don't have to - and keeping this ParsedGraphQLSchema as small as possible
    • Obviously in the previous implementation there is a lot of cloning of these TypeDefinitions
  • We'll also want to be able to Serialize/Deserialize this ParsedGraphQLSchema

@lostman lostman force-pushed the maciej/1157-refactor-graphql-schema branch from 12537fb to 4d06dce Compare August 21, 2023 12:12
@lostman
Copy link
Contributor Author

lostman commented Aug 21, 2023

@ra0x3, if we want to keep it as small as possible, we can remove the following:

ast: ServiceDocument

It is used in only one place:

    for definition in schema.ast().definitions.iter() {
        if let Some(def) = process_definition(&schema, definition) {
            output = quote! {
                #output
                #def
            };
        }
    }

And process_definition produces an output only for Types

    match definition {
        TypeSystemDefinition::Type(def) => process_type_def(schema, &def.node),
        TypeSystemDefinition::Schema(_def) => None,
        def => {
            panic!("Unhandled definition type: {def:?}");
        }
    }

If we want to throw any errors, like the above panic in the case of a Directive, we can do that at the parser level.

We could instead refer to type_defs directly:

    for (_, type_definition) in schema.type_defs().iter() {
        if let Some(def) = process_type_def(&schema, type_definition) {
        }
    }

Serialize/Deserialize

This poses a problem. The types we use from async-graphql-parser (e.g., TypeDefinition) are not serializable. We would have to write a serializer by hand.

Do we really need to serialize this struct? Loading the schema from the DB and parsing it shouldn't be a bottleneck. And for in-memory cache, we shouldn't need to serialize them.

One more thing, in addition to the ServiceDocument, we keep the raw schema String inside the ParsedGraphQL struct. It isn't used anywhere—only the version. So we should only keep the version and the parsed content. I did this in this PR, and I think it is the way to go, but I can always undo this change if needed.

@lostman lostman self-assigned this Aug 22, 2023
@lostman lostman marked this pull request as ready for review August 22, 2023 10:56
@lostman lostman requested review from ra0x3 and deekerno as code owners August 22, 2023 10:56
Copy link
Contributor

@ra0x3 ra0x3 left a comment

Choose a reason for hiding this comment

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

I think this does look cleaner. Will defer to @deekerno

Copy link
Contributor

@deekerno deekerno left a comment

Choose a reason for hiding this comment

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

LGTM!

@ra0x3 ra0x3 merged commit cfbfdab into develop Aug 29, 2023
@ra0x3 ra0x3 deleted the maciej/1157-refactor-graphql-schema branch August 29, 2023 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants