Skip to content

Commit

Permalink
Remove excessive cache inside buildClientSchema (#1677)
Browse files Browse the repository at this point in the history
  • Loading branch information
@IvanGoncharov
IvanGoncharov committed Jan 21, 2019
1 parent f1f1730 commit 183ff32
Showing 1 changed file with 15 additions and 26 deletions.
41 changes: 15 additions & 26 deletions src/utilities/buildClientSchema.js
View file
Expand Up @@ -7,9 +7,9 @@
* @flow strict
*/

import objectValues from '../polyfills/objectValues';
import inspect from '../jsutils/inspect';
import invariant from '../jsutils/invariant';
import keyMap from '../jsutils/keyMap';
import keyValMap from '../jsutils/keyValMap';
import { valueFromAST } from './valueFromAST';
import { parseValue } from '../language/parser';
Expand Down Expand Up @@ -84,19 +84,18 @@ export function buildClientSchema(
// Get the schema from the introspection result.
const schemaIntrospection = introspection.__schema;

// Converts the list of types into a keyMap based on the type names.
const typeIntrospectionMap = keyMap(
// Iterate through all types, getting the type definition for each.
const typeMap = keyValMap(
schemaIntrospection.types,
type => type.name,
typeIntrospection => typeIntrospection.name,
typeIntrospection => buildType(typeIntrospection),
);

// A cache to use to store the actual GraphQLType definition objects by name.
// Initialize to the GraphQL built in scalars. All functions below are inline
// so that this type def cache is within the scope of the closure.
const typeDefCache = keyMap(
specifiedScalarTypes.concat(introspectionTypes),
type => type.name,
);
for (const stdType of [...specifiedScalarTypes, ...introspectionTypes]) {
if (typeMap[stdType.name]) {

This comment has been minimized.

Sign in to view

Copy link
@martijnwalraven

martijnwalraven Mar 30, 2019

Contributor

We're running into an issue because it seems buildClientSchema would previously add built-in scalar types even if they weren't part of the introspection result. With these code changes, these types are only added if they are already part of the result. Was the previous behavior an unintended consequence, or should we get rid of this conditional to always add these types as well?

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov Mar 30, 2019

Author Member

Was the previous behavior an unintended consequence,

@IvanGoncharov I'm not an author of original code so I can't say for sure, but I'm pretty certain it was an unintended behavior.
Because schema constructed like this:

const schema = buildSchema(`
  type Query {
    foo: String
  }
`);

Will miss ID and Float types, so why should buildClientSchema behave differently.
Also it mean originIntrospection !== introspectionFromSchema(buildClientSchema(originIntrospection)).

or should we get rid of this conditional to always add these types as well?

So probably it make sense to remove if and revert to previous behaviour as 14.2.1 and do breaking change as 15.0.0.

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov Mar 31, 2019

Author Member

@martijnwalraven I released 14.2.1 with the fix included.

This comment has been minimized.

Sign in to view

Copy link
@abernix

abernix May 21, 2019 •
edited

@IvanGoncharov 👋 I just ran into this when git bisecting a peculiar bug which was tracking down when two additional types were suddenly present on an introspection result which were not present before.

That git bisect-ing led me to 3c79bed which, in turn, led me to this discussion!

Is your comment above meant to suggest that as of the fix in 14.2.1 that the following assertion should (once again?) be true?

originIntrospection === introspectionFromSchema(buildClientSchema(originIntrospection))

I believe what I'm finding/debugging is that this is not proving to be true, though it did appear to work prior to 3c79bed.

Reproduction (which I'm happy to open in a new issue, if this seems off):

const graphql = require("graphql");

const result = graphql.execute(
  graphql.buildSchema(`type Query { value: String }`),
  graphql.parse(graphql.getIntrospectionQuery())
);

// `false` in 3c79bed
// `true` in 3c79bed~ (before it)
// `true` in `v14.2.0` (tag)
// `true` in `v14.1.0` (tag)
// `true` in `v14.0.2` (tag)
JSON.stringify(result.data) ===
  JSON.stringify(
    graphql.introspectionFromSchema(
      graphql.buildClientSchema(result.data)
    )
  );

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov May 21, 2019

Author Member

@abernix Yes, this is was intentional 14.2.1 reverts pre-14.2.0 behavior where buildClientSchema contains all standard scalars. Since technically it's a breaking change and as @martijnwalraven pointed out it changes the behavior of graphql-js clients.
The plan is to fix it as part of upcoming 15.0.0, see #1809 with the proposed change.

Is it something critical and can you wait ~1month for 15.0.0 release?

This comment has been minimized.

Sign in to view

Copy link
@abernix

abernix May 21, 2019

@IvanGoncharov Hmm, I may be missing something, or this is a different breaking change to 14.2.1 which also changes the behavior of buildClientSchema.

reverts pre-14.2.0 behavior where buildClientSchema contains all standard scalars.

Despite what @martijnwalraven has mentioned above, I am not seeing all standard scalars present in pre-14.2.0 (inclusive of 14.2.0, actually). I am seeing all standard scalars present in 14.2.1, and the reproduction I've provided above seems to support that.

The behavior prior to 3c79bed (which is the only change in 14.2.1, other than the package version bump commit) was the same as 14.2.0. It was also the same as 14.1.0, was the same as 14.0.2, was the same as 14.0.1. I didn't check pre-14.x, but I suspect it may be the same there as well.

Looking into 3c79bed, it doesn't look like it is a straight revert of 183ff32, though I certainly understand there being reasons why that couldn't be directly reverted. 😄 (Tangentially, I did a more complete of that in my own abernix@4c86710. It fixed the problem but doesn't get rid of the excessive caching that you had set out to fix.).

I do think your solution in #1809 is correct, though currently this is a bit problematic (for us, at least) because when comparing client and server schemas (say from a client that introspects a server), these additional properties become present. Since we use the results of introspection for comparison (and SHA digest hashing, via stable stringification), this is causing undesired behavior.

I guess I'm looking for verification into the the additional scalar types were present prior to 14.2.1 because I'm certainly not seeing that.

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov May 21, 2019

Author Member

@abernix It looks like you are correct.
I modified your example to print scalar names:

const graphql = require("graphql");

const result = graphql.execute(
  graphql.buildSchema(`type Query { value: String }`),
  graphql.parse(graphql.getIntrospectionQuery())
);

result.data.__schema.types
  .filter(type => type.kind === 'SCALAR')
  .map(type => console.log(type.name));
console.log('=======================');
const schema = graphql.buildClientSchema(result.data);
Object.values(schema.getTypeMap())
  .filter(type => graphql.isScalarType(type))
  .map(type => console.log(type.name));

And before 14.2.1 it always print only String and Boolean.
So it looks like I made a fix before I confirmed the bug and later wrote tests for new behavior 🤦‍♂

@martijnwalraven It looks like we both can't reproduce the issue from your original comment. If it still possible can you please share a code sample?

This comment has been minimized.

Sign in to view

Copy link
@martijnwalraven

martijnwalraven May 22, 2019

Contributor

I don't have a reproduction unfortunately, but this was the error message that triggered me to post the original comment:
image (1)

This comment has been minimized.

Sign in to view

Copy link
@martijnwalraven

martijnwalraven May 22, 2019

Contributor

(I copied this from a Slack message that @trevor-scheer sent)

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov May 22, 2019

Author Member

@martijnwalraven It's very strange it's Boolean since boolean is used in introspection types and should be always present inside introspection.

What do you think can I merge #1809 and include it in the next release?

This comment has been minimized.

Sign in to view

Copy link
@martijnwalraven

martijnwalraven May 22, 2019

Contributor

It seems like that's the behavior we want, but I'd like to understand what could be going on with that error. Could it be that something changed with introspection itself? Are introspection results guaranteed to include the scalars that are used in the introspection types, even if these aren't used in the user's schema?

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov May 22, 2019

Author Member

Are introspection results guaranteed to include the scalars that are used in the introspection types, even if these aren't used in the user's schema?

Yes, it's guaranteed. Since you can get Boolean as a result of user query it should be inside introspection. Moreover Boolean is used in @skip & @include and it means the user should be able to define query variables of Boolean type.

Could it be that something changed with introspection itself?

As I understand this error is related to schema federation and introspection was sent from one of the microservices. So I have a couple of theories:

  • GraphQL on this particular microservice was implemented not with graphql-js but with some other library
  • Since introspection types are duplicated in each microservice and can conflict during merging maybe you are doing some kind of filtering to remove them.

This comment has been minimized.

Sign in to view

Copy link
@martijnwalraven

martijnwalraven May 22, 2019

Contributor

As far as I know, this error was not related to schema federation, and the client:check command just acts on a single project. I'm not entirely sure if the engine service is the underlying graphql-java service or our existing schema stitched gateway though. @abernix will probably know.

This comment has been minimized.

Sign in to view

Copy link
@abernix

abernix May 22, 2019 •
edited

@IvanGoncharov I don't think federation was in play here, but I can't be sure. I looked at it a bit with @trevor-scheer today and couldn't conclude anything.

It sounds like the original hunch here could have been incorrect and we need to put together a better reproduction. At the current moment, I don't think anyone on the team has the time to figure out how exactly this missing Boolean type was happening under those particular conditions.

As to what to do here and now given this information: graphql@14.2.1 was released at the end of March, but prior to that, this was the "norm" for ages and one could say that this 14.2.1 behavior differs from most other versions of graphql that have ever existed.

There's of course a philosophical question of whether undoing a breaking change that was only realized two months after it's released is another breaking change or not? Personally, I would undo the breaking change still and maintain relatively consistent behavior throughout the 14.x series. But at this point, I think the choice is yours. 😄

In the end, apologies for any misdirection that may have occurred here.

This comment has been minimized.

Sign in to view

Copy link
@IvanGoncharov

IvanGoncharov May 22, 2019

Author Member

@abernix No problem and big thanks for fast investigation 👍
For me, it's very important that issues and feature request were reported to this repo instead of being hacked/patched so false positives are inevitable in this process.
So please report issues if you are reasonably certain it's a problem with graphql-js even if you don't have the full reproduce example.

There's of course a philosophical question of whether undoing a breaking change that was only realized two months after it's released is another breaking change or not?

Semantic versioning is pretty straightforward about this:

As soon as you realize that you’ve broken the Semantic Versioning spec, fix the problem and release a new minor version that corrects the problem and restores backwards compatibility.

So I merged #1809.

This comment has been minimized.

Sign in to view

Copy link
@abernix

abernix May 22, 2019

Thanks, @IvanGoncharov! I'm glad they took the philosophy out of it. 😁
typeMap[stdType.name] = stdType;
}
}

// Given a type reference in introspection, return the GraphQLType instance.
// preferring cached instances before building new instances.
Expand All @@ -123,20 +122,16 @@ export function buildClientSchema(
}

function getNamedType(typeName: string): GraphQLNamedType {
if (typeDefCache[typeName]) {
return typeDefCache[typeName];
}
const typeIntrospection = typeIntrospectionMap[typeName];
if (!typeIntrospection) {
const type = typeMap[typeName];
if (!type) {
throw new Error(
`Invalid or incomplete schema, unknown type: ${typeName}. Ensure ` +
'that a full introspection query is used in order to build a ' +
'client schema.',
);
}
const typeDef = buildType(typeIntrospection);
typeDefCache[typeName] = typeDef;
return typeDef;

return type;
}

function getInputType(typeRef: IntrospectionInputTypeRef): GraphQLInputType {
Expand Down Expand Up @@ -362,12 +357,6 @@ export function buildClientSchema(
});
}

// Iterate through all types, getting the type definition for each, ensuring
// that any type not directly referenced by a field will get created.
const types = schemaIntrospection.types.map(typeIntrospection =>
getNamedType(typeIntrospection.name),
);

// Get the root Query, Mutation, and Subscription types.
const queryType = schemaIntrospection.queryType
? getObjectType(schemaIntrospection.queryType)
Expand All @@ -392,7 +381,7 @@ export function buildClientSchema(
query: queryType,
mutation: mutationType,
subscription: subscriptionType,
types,
types: objectValues(typeMap),
directives,
assumeValid: options && options.assumeValid,
allowedLegacyNames: options && options.allowedLegacyNames,
Expand Down

0 comments on commit 183ff32

Please sign in to comment.