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

feat: Ignore sorting for queries and mutations when printing schema #2362

Closed
wants to merge 2 commits into from

Conversation

wtrocki
Copy link
Contributor

@wtrocki wtrocki commented Jan 19, 2020

Motivation

When using printSchema it's implementation will automatically sort all types by names.
While for user types scalars and inputs this makes sense, for very large schemas finding mutation, query etc. can be challenging without using find command. Working with that schema layout is also hard as it is usually better to have mutations and queries together.

I heard opinions from the community that would expect schema to be organized the same way as it was before printing was applied. Print schema is often used by some CLI tools where we just want to append some specific object to the schema while operating on ast objects rather than strings. It will be amazing to see print schema being used more for those use cases.

How this could be done

Always put queries/mutations on the bottom of printed schema

This is implemented in this PR as I thought that it will be the least controversial change, but it looks like it is causing failures to some tests that rely on print schema order.
I wanted to make sure that the community will get that behavior out of the box without using some advanced functions etc. IMHO Scalars and directives could be also reorganized

Allow developers to apply their own sorting function

Currently, the order of the types in schema is not preserved as they are stored in map.
What it means is if users organized schema to their liking in DSL and then we parsed that into object original order is lost (not 100% sure about this but this is what I see happening)

To be able to get some organization when printing schema an separate sorting function could be applied. For example, first list scalars scalars and directives. Then user types in alphabetical order. Then Queries, Mutations. Subscriptions.

This can be done by adding additional parameter to options or exposing printFilteredSchema for advanced use cases

@wtrocki wtrocki requested a review from IvanGoncharov January 19, 2020 13:09
@cmwylie19
Copy link

Excellent enhancement 👍

@wtrocki
Copy link
Contributor Author

wtrocki commented Jan 19, 2020

The build is failing due to not updated unit tests snapshots. I will hold off with updates to not obfuscate the change that was done

@pratiklodha
Copy link

Yes. This is a very good enhancement.

@wtrocki
Copy link
Contributor Author

wtrocki commented Jan 21, 2020

I totally forgot that schema will contain loc fields that can be used to fully reconstruct order of items. Using loc would be much better than putting queries on the bottom etc.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jan 23, 2020

@wtrocki Thanks for PR 👍
Totally agree that it's something we need to address.

I totally forgot that schema will contain loc fields that can be used to fully reconstruct order of items. Using loc would be much better than putting queries on the bottom etc.

Only schemas built from SDL has loc 😞
But in many cases people try to print remote schema s that are built with buildClientSchema that doesn't have AST nodes at all.
Both SDL and introspection (because types are sent as an array) have an order we just need to preserve it during GraphQLSchema construction.

I experimented a bit with a few different approaches and found one that keeps the order intact without introducing the performance penalty and I'm working on PR.
So my proposal is to remove type sorting completely from printSchema.
But if someone needs to work with the non-stable schema you can always do printSchema(lexicographicSortSchema(schema)) instead.
What do you think?

@wtrocki
Copy link
Contributor Author

wtrocki commented Jan 23, 2020

Thank you so much for reaching out. I have done some and found out that order will be preserved when iterating using Object.keys

Note: Since ECMAScript 2015, objects do preserve creation order for string and Symbol keys. In JavaScript engines that comply with the ECMEScript 2015 spec, iterating over an object with only string keys will yield the keys in order of insertion.

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

So my proposal is to remove type sorting completely from printSchema.

Basing on what I see that could be the best way to get this.
There is simply no need to filter things out.
A very simplified version of printing could look as follows:

https://gist.github.com/wtrocki/df0f9cdca8f3c41d4d607a57a211e68f

printSchema(lexicographicSortSchema(schema)) instead.

That will be also a good alternative.

Edit:
I have made this trivial change to see how many things will break in the tests and implementation. If there is need to make this not breaking we could introduce another flag in print options.

@wtrocki wtrocki force-pushed the fitering-schema-print branch from 73d8fee to c174dc8 Compare January 23, 2020 09:58
@wtrocki wtrocki force-pushed the fitering-schema-print branch from c174dc8 to 0912c06 Compare January 23, 2020 09:59
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 29, 2020
IvanGoncharov added a commit that referenced this pull request Jan 29, 2020
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 29, 2020
Fixes graphql#2362

It's a step toward having reversable `buildSchema`:
```
sdl === printSchema(buildSchema(sdl))
```
At the same time if you need fully predictable SDL output (e.g. to diff
schemas) you can achieve this using `llexicographicSortSchema`:
```
printSchema(lexicographicSortSchema(schema))
```

But if some reason you can't use `lexicographicSortSchema` please open
an issue and describe your use case in more details.
IvanGoncharov added a commit that referenced this pull request Jan 29, 2020
Fixes #2362

It's a step toward having reversable `buildSchema`:
```
sdl === printSchema(buildSchema(sdl))
```
At the same time if you need fully predictable SDL output (e.g. to diff
schemas) you can achieve this using `llexicographicSortSchema`:
```
printSchema(lexicographicSortSchema(schema))
```

But if some reason you can't use `lexicographicSortSchema` please open
an issue and describe your use case in more details.
@IvanGoncharov
Copy link
Member

@wtrocki Thank you for taking time to write a detailed description and PoC.
Fixed in #2411 and will be released as 15.0.0-rc.2

abernix added a commit to apollographql/apollo-server that referenced this pull request Feb 17, 2020
The order of types returned by, e.g. `getImplementation`, will now be
dependent on the order that those types first appear in the schema.

See the referenced issues, the first of which implemented the change and the
second which indicates the motivation.

Ref: graphql/graphql-js#2410
Ref: graphql/graphql-js#2362
abernix added a commit to apollographql/apollo-server that referenced this pull request Feb 24, 2020
The order of types returned by, e.g. `getImplementation`, will now be
dependent on the order that those types first appear in the schema.

See the referenced issues, the first of which implemented the change and the
second which indicates the motivation.

Ref: graphql/graphql-js#2410
Ref: graphql/graphql-js#2362
nodkz added a commit to graphql-compose/graphql-compose that referenced this pull request Mar 6, 2020
abernix added a commit to apollographql/apollo-server that referenced this pull request May 5, 2020
The order of types returned by, e.g. `getImplementation`, will now be
dependent on the order that those types first appear in the schema.

See the referenced issues, the first of which implemented the change and the
second which indicates the motivation.

Ref: graphql/graphql-js#2410
Ref: graphql/graphql-js#2362
abernix added a commit to apollographql/apollo-server that referenced this pull request May 5, 2020
The order of types returned by, e.g. `getImplementation`, will now be
dependent on the order that those types first appear in the schema.

See the referenced issues, the first of which implemented the change and the
second which indicates the motivation.

Ref: graphql/graphql-js#2410
Ref: graphql/graphql-js#2362
abernix added a commit to apollographql/apollo-server that referenced this pull request Jun 8, 2020
In a general sense this just updates tests to accommodate the new error
conditions and schema hashes since a lot of the work has already been done
in #3712 / 131c9b8.

A larger note is due to explain the updating the snapshots.  Those changes
are due the fact that types returned by, e.g.  `getImplementation`, will now
be dependent on the order that those types first appear in the schema.

See the referenced issues, the first of which implemented the change and the
second which indicates the motivation.

Ref: graphql/graphql-js#2410
Ref: graphql/graphql-js#2362
Ref: #3712 (131c9b8)
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 this pull request may close these issues.

4 participants