More allowed legacy names #1235

alloy · 2018-02-08T23:18:24Z

Since yesterday I went back and did some more code reviewing and realised there’s another few situations where the allowed legacy names configuration isn’t being passed on, apologies for not catching that yesterday.

alloy · 2018-02-08T23:21:10Z

src/type/schema.js

    }

+    this.__allowedLegacyNames = config.allowedLegacyNames;


The assumeValid configuration is about whether or not to check the configuration for common mistakes, so regardless of that the allowed legacy names still need to be stored on the schema instance.

There’s no tests for assumeValid and the below configuration being set or not, so wasn’t sure what you’d like. Should I add a test for any configuration being set, regardless of assumeValid?

Adding a test would be great!

Let me know if I understand this correctly - you could create a GraphQLSchema with both __allowedLegacyNames and assumeValid, then extend that schema with a legacy name (which might no longer be assumed invalid), and then expect validation to pass?

Hmm, I now see that validate depends on the private __validationErrors property of a schema instance.

I’m now starting to think that maybe what Relay is doing isn’t entirely correct. First it creates a schema with assumeValid set, then after extending that schema a few times it expects to still be able to validate that schema, which appears to only really be possible because extendSchema does not copy over the assumeValid configuration.

Maybe extendSchema should also copy over the assumeValid setting, but validate could accept an optional parameter to force performing the validations regardless of __validationErrors already being set?

Not quite. extendSchema produces a new schema - while the provided schema may have been "valid" (either assumed or validated), the returned schema is different and may be invalid, so it must not copy over assumeValid or __validationErrors from the input. We would expect the extended schema to be validated by validateSchema().

Now, we may want to provide a similar assumeValid option for extendSchema, but that would be a separate thing.

validate checks __validationErrors first as a form of memoization. Since schema are considered immutable, we do not want to wastefully validate the same schema multiple times.

Relay first builds a schema with the assumeValid set, because it expects that schema has been provided from a valid GraphQL server - whereas any client side extensions are within Relay's responsibility to validate

Also, the GraphQLValidator second link is validating the graphql operations and fragments found within relay product code, not the schema

Ok, I see, that makes sense. So to answer your initial question, when an original schema has __allowedLegacyNames then extending that schema should still allow those original legacy names too, regardless from whether or not they were validated on the original schema.

That validate call in GraphQLValidator is definitely still leading to an error about the legacy name in the schema . Maybe it’s just a side-effect, though, I’ll double-check that in the morrow.

Probably a side effect - the document validator first asserts that a valid schema if being used to validate a document. If the schema wasn't validated before that point, it will throw

leebyron · 2018-02-08T23:27:01Z

src/utilities/extendSchema.js

+   * This option is provided to ease adoption and may be removed in a future
+   * major release.
+   */
+  allowedLegacyNames?: ?Array<string>,


Should this be added here? If so, should the result of extendSchema include both the allowed legacy names of the input schema in addition to these?

Good point, that seems like an excellent idea 👍

leebyron · 2018-02-08T23:27:55Z

src/utilities/buildASTSchema.js

+   * This option is provided to ease adoption and may be removed in a future
+   * major release.
+   */
+  allowedLegacyNames?: ?Array<string>,


This makes sense as an alternate API for constructing a schema. 👍

IvanGoncharov · 2018-02-09T00:15:47Z

@alloy @leebyron In theory buildClientSchema also should accept allowedLegacyNames.

Idea mode: Can we create a global array in assertValidName.js and just export addAllowedLegacyName function. We guarantee to have only one instance of graphql so you could write:

import { addAllowedLegacyName } from 'graphql';

addAllowedLegacyName('__badname');
someRelayFunctionWorkingWithSDL(`
  type Query {
     __badname: String
  }
`);

That way we minimize changes in graphql-js to one file and allow to use the relay and other 3rd-party libs without any changes.
What do you think?

alloy · 2018-02-09T16:04:47Z

@IvanGoncharov I’m personally not a big fan of singletons, but have no idea if that pattern is already being employed in this library or if that’s something that’s acceptable. Will defer to @leebyron on that.

What I did do is consolidate the schema validation options into a single type that’s shared and added the allowedLegacyNames option to buildClientSchema as well.

alloy · 2018-02-09T18:42:16Z

Oh, just remembered that I still need to add tests for the existing assumeValid behaviour. Will do that later today.

IvanGoncharov · 2018-02-09T21:11:59Z

src/utilities/buildClientSchema.js

-   * Default: false
-   */
-  assumeValid?: boolean,
-|};


I still think it's better to have type Options = {| ...GraphQLSchemaValidationOptions |} even though there are no other options at the moment. It shows that options arg could be used for some other options beyond validation.

Gotcha, makes sense 👍

alloy · 2018-02-09T22:29:53Z

@leebyron Alright, I think I’ve addressed all feedback.

leebyron

This is looking good - some smaller suggestions inline

leebyron · 2018-02-14T20:17:09Z

src/utilities/__tests__/extendSchema-test.js

@@ -1225,18 +1225,23 @@ describe('extendSchema', () => {
      query: new GraphQLObjectType({
        name: 'Query',
        fields: () => ({
-          id: { type: GraphQLID },
+          __badName: { type: GraphQLString },


Could you add a new test rather than changing this existing one?

leebyron · 2018-02-14T20:20:17Z

src/utilities/extendSchema.js

+  ];
+  if (allowedLegacyNames.length === 0) {
+    allowedLegacyNames = null;
+  }


This merge code could be much simpler and avoid the mutable var:

const schemaAllowedLegacyNames = schema.__allowedLegacyNames const extendAllowedLegacyNames = options && options.allowedLegacyNames const allowedLegacyNames = schemaAllowedLegacyNames && extendAllowedLegacyNames ? schemaAllowedLegacyNames.concat(extendAllowedLegacyNames) : schemaAllowedLegacyNames || extendAllowedLegacyNames

alloy · 2018-02-14T23:28:28Z

src/utilities/extendSchema.js

+  const allowedLegacyNames = 
+    schemaAllowedLegacyNames && extendAllowedLegacyNames
+      ? schemaAllowedLegacyNames.concat(extendAllowedLegacyNames)
+      : schemaAllowedLegacyNames || extendAllowedLegacyNames;


alloy added 2 commits February 9, 2018 00:07

Allow legacy names when building AST schema.

5cb3dd9

Always store allowed legacy names on schema.

5f9ff08

facebook-github-bot added the CLA Signed label Feb 8, 2018

alloy commented Feb 8, 2018

View reviewed changes

leebyron reviewed Feb 8, 2018

View reviewed changes

Allow additional legacy names when extending a schema.

3958eae

Standardise schema validation options.

8f936da

IvanGoncharov reviewed Feb 9, 2018

View reviewed changes

alloy added 2 commits February 9, 2018 23:18

Allow legacy names in client schemas.

bb9c68c

Add test coverage for schema validation options.

3beb355

alloy force-pushed the more-allowed-legacy-names branch from a6e620f to 3beb355 Compare February 9, 2018 22:19

leebyron requested changes Feb 14, 2018

View reviewed changes

leebyron added 4 commits February 14, 2018 15:31

Simplify value merge

50be8c6

Add test

e58be0f

Trailing space

ae1ab1d

Recycling this value means we need to be read-only strict

e8b6262

leebyron merged commit b4f2955 into graphql:master Feb 14, 2018

alloy commented Feb 14, 2018

View reviewed changes

alloy deleted the more-allowed-legacy-names branch February 14, 2018 23:28

This was referenced Feb 16, 2018

GraphQL interface type and class extending support MichalLytek/type-graphql#6

Closed

Adding in allOf functionality yarax/swagger-to-graphql#41

Closed

This was referenced Mar 19, 2018

Bump graphql from 0.13.0 to 0.13.2 Thorium-Sim/thorium#619

Closed

Bump graphql from 0.12.3 to 0.13.2 jamesabels/graphql-bp#17

Closed

build: bump graphql from 0.10.5 to 0.13.2 hongymagic/k2#160

Open

snyk-bot mentioned this pull request Mar 3, 2020

[Snyk] Upgrade graphql from 0.13.1 to 0.13.2 SalesforceCommerceCloud/sfcc-sample-apps#132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More allowed legacy names #1235

More allowed legacy names #1235

alloy commented Feb 8, 2018

alloy Feb 8, 2018

leebyron Feb 8, 2018

alloy Feb 8, 2018

leebyron Feb 8, 2018

leebyron Feb 8, 2018

leebyron Feb 8, 2018

alloy Feb 9, 2018

alloy Feb 9, 2018

leebyron Feb 9, 2018

leebyron Feb 8, 2018

alloy Feb 8, 2018

leebyron Feb 8, 2018

IvanGoncharov commented Feb 9, 2018

alloy commented Feb 9, 2018

alloy commented Feb 9, 2018

IvanGoncharov Feb 9, 2018

alloy Feb 9, 2018

alloy commented Feb 9, 2018

leebyron left a comment

leebyron Feb 14, 2018

leebyron Feb 14, 2018

alloy Feb 14, 2018

More allowed legacy names #1235

More allowed legacy names #1235

Conversation

alloy commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Feb 9, 2018

alloy commented Feb 9, 2018

alloy commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alloy commented Feb 9, 2018

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment