From c38a8a1d2a4ca42aa87dbfd3abb5f23fbf3eff0d Mon Sep 17 00:00:00 2001 From: Greg Hurrell Date: Thu, 16 Feb 2017 09:31:39 -0800 Subject: [PATCH 1/3] Use console.warn in name validation In fe346190671c4ad0d666e8eef 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. --- src/type/__tests__/validation-test.js | 6 +++--- src/utilities/assertValidName.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index 71b70e1fa5..7243cdbf29 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -356,9 +356,9 @@ describe('Type System: Objects must have fields', () => { it('warns about an Object type with reserved named fields', () => { /* eslint-disable no-console */ - const realConsoleError = console.error; + const realConsoleWarn = console.warn; const calls = []; - console.error = function () { + console.warn = function () { calls.push(Array.prototype.slice.call(arguments)); }; try { @@ -371,7 +371,7 @@ describe('Type System: Objects must have fields', () => { 'Name "__notPartOfIntrospection" must not begin with "__", which is reserved by GraphQL introspection.' ); } finally { - console.error = realConsoleError; + console.warn = realConsoleWarn; } /* eslint-enable no-console */ }); diff --git a/src/utilities/assertValidName.js b/src/utilities/assertValidName.js index ce63ae6965..d279db16b8 100644 --- a/src/utilities/assertValidName.js +++ b/src/utilities/assertValidName.js @@ -28,12 +28,12 @@ export function assertValidName( if (!isIntrospection && name.slice(0, 2) === '__' && !hasWarnedAboutDunder) { hasWarnedAboutDunder = true; /* eslint-disable no-console */ - if (console && console.error) { + if (console && console.warn) { const error = new Error( `Name "${name}" must not begin with "__", which is reserved by ` + 'GraphQL introspection.' ); - console.error(error.stack || String(error)); + console.warn(error.stack || String(error)); } /* eslint-enable no-console */ } From 37d4521795646c82899e85544075c5c1aaada8ee Mon Sep 17 00:00:00 2001 From: Greg Hurrell Date: Thu, 16 Feb 2017 11:36:38 -0800 Subject: [PATCH 2/3] 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. --- .../__tests__/assertValidName-test.js | 139 ++++++++++++++++++ src/utilities/assertValidName.js | 20 ++- 2 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 src/utilities/__tests__/assertValidName-test.js diff --git a/src/utilities/__tests__/assertValidName-test.js b/src/utilities/__tests__/assertValidName-test.js new file mode 100644 index 0000000000..cabd54ba64 --- /dev/null +++ b/src/utilities/__tests__/assertValidName-test.js @@ -0,0 +1,139 @@ +/** + * Copyright (c) 2015, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import { describe, it } from 'mocha'; +import { expect } from 'chai'; +import { formatWarning } from '../assertValidName'; + +/** + * Helper for dedenting indented template literals. This helps us to + * keep the tests pretty. + */ +function dedent(string) { + // Get lines, discarding empty leading and trailing lines. + const lines = string.replace(/^[ \t]*\n|\n[ \t]*$/g, '').split('\n'); + + // Find smallest indent. + const indent = lines.reduce((currentMinimum, line) => { + const whitespace = line.match(/^ +/); + return Math.min( + whitespace ? whitespace[0].length : 0, + currentMinimum + ); + }, Infinity); + + // Remove indent from each line. + return lines.map(line => line.slice(indent)).join('\n'); +} + +/** + * Convenience method for creating an Error object with a defined stack. + */ +function createErrorObject(message, stack) { + const error = new Error(message); + error.stack = stack; + return error; +} + +describe('formatWarning()', () => { + it('formats given a Chrome-style stack property', () => { + const chromeStack = dedent(` + Error: foo + at z (:1:21) + at y (:1:15) + at x (:1:15) + at :1:6 + `); + const error = createErrorObject('foo', chromeStack); + expect(formatWarning(error)).to.equal(dedent(` + foo + at z (:1:21) + at y (:1:15) + at x (:1:15) + at :1:6 + `)); + }); + + it('formats given a Node-style stack property', () => { + const nodeStack = dedent(` + Error: foo + at z (repl:1:29) + at y (repl:1:23) + at x (repl:1:23) + at repl:1:6 + at ContextifyScript.Script.runInThisContext (vm.js:23:33) + at REPLServer.defaultEval (repl.js:340:29) + at bound (domain.js:280:14) + at REPLServer.runBound [as eval] (domain.js:293:12) + at REPLServer.onLine (repl.js:537:10) + at emitOne (events.js:101:20) + `); + const error = createErrorObject('foo', nodeStack); + expect(formatWarning(error)).to.equal(dedent(` + foo + at z (repl:1:29) + at y (repl:1:23) + at x (repl:1:23) + at repl:1:6 + at ContextifyScript.Script.runInThisContext (vm.js:23:33) + at REPLServer.defaultEval (repl.js:340:29) + at bound (domain.js:280:14) + at REPLServer.runBound [as eval] (domain.js:293:12) + at REPLServer.onLine (repl.js:537:10) + at emitOne (events.js:101:20) + `)); + }); + + it('formats given a Firefox-style stack property', () => { + const firefoxStack = dedent(` + z@debugger eval code:1:20 + y@debugger eval code:1:14 + x@debugger eval code:1:14 + @debugger eval code:1:5 + `); + const error = createErrorObject('foo', firefoxStack); + expect(formatWarning(error)).to.equal(dedent(` + foo + z@debugger eval code:1:20 + y@debugger eval code:1:14 + x@debugger eval code:1:14 + @debugger eval code:1:5 + `)); + }); + + it('formats given a Safari-style stack property', () => { + const safariStack = dedent(` + z + y + x + global code + evaluateWithScopeExtension@[native code] + _evaluateOn + _evaluateAndWrap + evaluate + `); + const error = createErrorObject('foo', safariStack); + expect(formatWarning(error)).to.equal(dedent(` + foo + z + y + x + global code + evaluateWithScopeExtension@[native code] + _evaluateOn + _evaluateAndWrap + evaluate + `)); + }); + + it('formats in the absence of a stack property', () => { + const error = createErrorObject('foo'); + expect(formatWarning(error)).to.equal('foo'); + }); +}); diff --git a/src/utilities/assertValidName.js b/src/utilities/assertValidName.js index d279db16b8..a2837da5af 100644 --- a/src/utilities/assertValidName.js +++ b/src/utilities/assertValidName.js @@ -9,6 +9,7 @@ */ const NAME_RX = /^[_a-zA-Z][_a-zA-Z0-9]*$/; +const ERROR_PREFIX_RX = /^Error: /; // Ensures console warnings are only issued once. let hasWarnedAboutDunder = false; @@ -33,7 +34,7 @@ export function assertValidName( `Name "${name}" must not begin with "__", which is reserved by ` + 'GraphQL introspection.' ); - console.warn(error.stack || String(error)); + console.warn(formatWarning(error)); } /* eslint-enable no-console */ } @@ -43,3 +44,20 @@ export function assertValidName( ); } } + +/** + * Returns a human-readable warning based an the supplied Error object, + * including stack trace information if available. + */ +export function formatWarning(error: Error): string { + let formatted = ''; + const errorString = String(error).replace(ERROR_PREFIX_RX, ''); + const stack = error.stack; + if (stack) { + formatted = stack.replace(ERROR_PREFIX_RX, ''); + } + if (formatted.indexOf(errorString) === -1) { + formatted = errorString + '\n' + formatted; + } + return formatted.trim(); +} From 44a2cff1a5abd33c16222848bc1cad470777535a Mon Sep 17 00:00:00 2001 From: Greg Hurrell Date: Thu, 16 Feb 2017 11:40:37 -0800 Subject: [PATCH 3/3] 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. --- src/utilities/assertValidName.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utilities/assertValidName.js b/src/utilities/assertValidName.js index a2837da5af..636f55590c 100644 --- a/src/utilities/assertValidName.js +++ b/src/utilities/assertValidName.js @@ -32,7 +32,8 @@ export function assertValidName( if (console && console.warn) { 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 ' + + 'become a hard error.' ); console.warn(formatWarning(error)); }