Soften name validation warnings to avoid CI issues #717

Merged
merged 3 commits into from Feb 16, 2017

Projects

None yet

4 participants

@wincent
Contributor
wincent commented Feb 16, 2017

A few things here:

  • Downgrade console.error to console.warn.
  • Strip "Error: " prefix from messages that can trip up CIs.
  • Note in warning message that it will eventually become a hard error.
wincent added some commits Feb 16, 2017
@wincent wincent Use console.warn in name validation
In fe34619 we degraded the hard error (ie. a `throw`)
for non-compliant field names (ie. starting with `__`) to a
`console.error`.

We've found, however, that certain CI systems will treat the use of
`console.error` as a cause for failure, and our intent here is not to
block what would otherwise be a valid test run.

Switch to `console.warn` instead.

Eventually in a later release this will become a hard error, but for now
continuing to treat this like a "deprecation" and just warn is
appropriate.
c38a8a1
@wincent wincent Avoid "Error:" text in advisory warnings
This goes a step further than the parent commit, which downgrades
`console.error` to `console.warn` for cases that are really just
advisory "deprecation" warnings for non-compliant schemas.

The motivation in that commit was preventing CI failures stemming from
use of `console.error`.

That may not be enough though, because on some JS engines (notably
Chrome/Node), "Error: " still appears in the output thanks to our use of
`Error` objects to get stack traces. Depending on how the CI is set-up,
that could also be enough to spuriously fail the run.

So in this commit, we add a `formatWarning` helper that takes the
`Error` object and grooms it for use as a human-readable warning
message.

Added tests to capture and verify the different engine behaviors.
37d4521
@wincent wincent Note that failing dunderscore name validation will become a hard error
To adequately serve as a deprecation warning, the message should provide
advance notice that it non-compliance will eventually become a hard
error. I haven't put a version number in there yet because I don't know
which version it will happen in. Once we make that call we should make
the message more precise.
44a2cff
@wincent wincent requested review from leebyron and asiandrummer Feb 16, 2017
@leebyron
Contributor

Thanks for doing the research and adding tests. This looks great. Feel free to merge.

I think beyond this we should follow-up with with a slightly more invasive change to separate the construction of a schema from the validation of a schema.

When building a "source of truth" schema (e.g. you run a node.js server, and use GraphQL.js to present a GraphQL service) then we should encourage validation - strict validation - to ensure the schema is well formed. I'm not sure what the best way to accomplish this would be and am open to ideas. Perhaps execute() or other operations dependent on a valid schema would check some internally-set flag for "has been validated", and if it's not set, run validation? Just an idea.

When building a "reference" schema (e.g. you used introspection to learn about a schema, and have rebuilt it to power a tool like GraphiQL) then we have very different validation needs. Assumptions made by execution like an object adhering to its implemented interfaces, or (in this case) names being well formed may not break assumptions of the tools being used - we're both paying a computation cost to perform this validation and we're forcing tools to be more strict than they might need to be. Should GraphiQL stop working or report errors if it's used against a server that has an invalid schema? If it should, how do we define that line?

This diff is a great incremental step to easing the pain of this particular change, but it would be awesome to see a broader rethink of how schema validation fits into the whole story.

const error = new Error(
`Name "${name}" must not begin with "__", which is reserved by ` +
- 'GraphQL introspection.'
+ 'GraphQL introspection. In a future release of graphql this will ' +
@asiandrummer
asiandrummer Feb 16, 2017 Contributor

nit: graphql -> graphql-js?

@wincent
wincent Feb 16, 2017 Contributor

I went with graphql because that is the NPM package name. graphql-js is only the name of the GitHub repo.

@wincent
Contributor
wincent commented Feb 16, 2017

Split off #719 to carry out discussion about the deeper changes to the validation model.

@wincent wincent merged commit f7b94d1 into graphql:master Feb 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 97.994%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment