The Schema validation/merge is broken

[bboure]

Since #437, the schema validator and merger is broken.

The following errors started appearing:

AppSync Plugin: Error: There can be only one type named "Query".

There can be only one type named "Mutation".
There can be only one type named "Query".

The reason is that multiple files define Query and Mutation

Example:

appSync:
  schema: *.graphql

// users.graphql
type Query {
    getUser(id: ID!): User!
}

// posts.graphql
type Query {
    getPosts(id: ID!): Post!
}

Expected result:

Schema gets converted to

type Query {
    getUser(id: ID!): User!
    getPosts(id: ID!): Post!
}

What happens instead:

The above error.

@vicary

[bboure]

A second possible problem I found with appsync-schema-converter:

Literal comments do get transformed into # (expected behaviour), but all # comments get removed

Example:

"""
This comment will remain and be converted to #
"""
type Query {
}

# This comment will disappear
type mutation {
}

result

# This comment will remain and be converted to #
type Query {
}

type mutation {
}

We should probably support "old style" comments

[vicary]

@bboure I will see if the new grapgql.js supports merging types the old way, without the extends keyword.

Keeping the comments should be quick, I'll do some tests for both in the weekend.

[vicary]

Let me explain a bit more on the schema merging behavior, and how I am planning to solve this.

The Conflicting Approaches

1. The graphql-tools way
   It uses schema merging via @graphql-tools/merge#mergeTypeDefs which expects multiple instances of top level queries from separate files, for example

# User.graphql

type User {
  id: ID!
  username: String!
}

type Query {
  users: [User!]!
}

# Article.graphql

type Article {
  id: ID!
  author: User!
  content: String!
}

type Query {
  articles: [Article!]!
}

This method does NOT support the extend keyword below when I started appsync-schema-converter, I haven't tested again since then.

2. The graphql.js way
   It parses multiple concatenated files and extends existing types via the extend keyword, for example we simply concatenates the following files before conversion:

# root.graphql

type Query
type Mutation

# User.graphql

type User {
  id: ID!
  username: String!
}

extend type Query {
  users: [User!]!
}

# Article.graphql

type Article {
  id: ID!
  author: User!
  content: String!
}

extend type User {
  articles: [Article!]!
}

extend type Query {
  articles: [Article!]!
}

The Solution

1. We can still take in multiple files from serverless-appsync-plugin, merging the graphql-tools way; and tries to convert each individual files with appsync-schema-converter. This way we make both approaches compatible.
2. For the comments, there is in fact an exposed option called commentDescription. We may expose all printer options under custom.appSync, while keeping the old behavior as defaults.
   

What do you think?

[bboure]

I also had a look at this yesterday. I used a similar approach.

The way I got it to work (I think)was using the old behaviour first with mergeTypeDefs , then pass the result into appsync-schema-converter to "fix" comments, etc.

I need to read more doc and code and see what is the "recommended" usage for "schema stitching".
We definitely want to keep backward compatibility, but, FYI I am working on a newer version of this plugin.
I'd like to understand what is the best approach and introduce it as a breaking change in the next major version bump.

[vicary]

I think the best option is to support both versions at the same time.

You may make the final call on whether to force a choice behind an option, or simply support both ways with my proposed solution.

I am sharing the pieces of backstories I have learnt below, along with some of my opinions.

---

People coming from the Apollo ecosystem maybe confused by the legacy syntax in AppSync, and there are people started small from the Amplify toolchain. The community is fragmented.

I have read comments from many people about rooms for improvements from the GraphQL spec governance, all the way to the fact that AppSync refuses to catch up with it.

graphql-tools rushed out a merging solution mergeTypes before extend was a thing, looking from the perspective of an outsider, I feel a strong influence from Apollo.

Apollo is trying so hard to get people to pipe their production data to their platform, see Apollo Engine, Apollo Studio (adapted from GraphiQL), and their latest boy, Schema Federation. I think this makes schema stitching less of an incentive to be advertised and promoted.

While graphql.js is the most recent and "canonical" way of spec implementation, the GraphQL discord server now houses everyone under the same community. They are trying to defrag, we'll see.

With the current situation, I think it is unlikely for AppSync to support newer version of specs without introducing something new, similar to API Gateway V2 (HTTP API) vs the original API Gateway (REST API).

If by any means possible, I'd love to see this plugin embraces both sides of an otherwise unfortunately fragmented user base.

[bboure]

The idea would to support both ways: either use "standard" graphql or "the AppSync way" (with # descriptions, and ", " separators)

The problem is that the tooling does not support AppSync schemas and it's hard to work with it in the plugin. eg: for validation, stitching, etc

By default, I would advocate for using "standard" GraphQL and "translate" under the hood.
If AppSync ends up catching up, it would make migrations easier. But indeed, they would need to introduce at least a "version" option or something to avoid breaking changes.

[vicary]

If we are going the direction of either-or, I can make a PR to introduce a new option legacySchema: true, or, the other way round, modernSchema: false under custom.appSync.

For now it defaults to the legacy way for backward compatibility, we can change the default for the next major release.

[bboure]

I have been investigating this yesterday for a while and I think there is no easy fix to this.

Here are my findings:

It looks like AppSync now handles schemas better:

• & are accepted and converted to , automatically
• “”” are also accepted but are removed
• enum comments still don’t work properly
  • They are removed at best
  • AppSync sometimes throws some errors

I contacted the AppSync team and it looks like they are working on improving this even further.

More details here

Also, mergeTypeDefs transforms """ descriptions into comments (#).

Given this, in order to solve your issue (accept """ descriptions), I suggest that we fix this the following way in v1.

• Rollback PR Better converts and preserves comment descriptions #437
• recommend using the allowHashDescription flag in order to keep comments. It should work also with """.

I tested this and it works. This is the solution that has the less impact on compatibility

---

In v2 I suggest the following:

• Remove mergeTypeDefs
• Recommend the usage of extend to extend/split schemas in several files
  • Schema will be stitched together by concatenating files
  • AppSync does not support extend yet, but I think we can use parse and print from graphql-js to rebuild a compatible schema (extended types should be merged)
  • If the config is only contained in one single file, do not try to "fix" anything, just send the file as-is to AppSync
• Do not try to fix anything else
  • """ descriptions will be kept as-is, they will be removed by AppSync later
  • & will be kept as-is, they will be transformed into , by AppSync later
  • # Comments will be removed by parse + print. Support for it might come later but until then, they will not be supported.

If we do the above and hope that AppSync catches up with the standards soon enough, we would not need to change almost anything to the implementation and I don't think there would be any breaking change.

LMK what you think or if I missed something.

Thanks

@vicary

Edit: I think the idea would be to enforce modern schemas in v2. (e.g.: NOT accept "," for interfaces).
This would allow us to be future-proof. Anything not compatible will either be fixed by AppSync or ignored/removed (as explained above). If AppSync improves compatibility in the future, this plugin would be ready.