diff --git a/CHANGELOG.md b/CHANGELOG.md index fdb5d4d3b..0ada103a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- GraphQL: restore lenient variable coercion for the built-in `Int`, `Float`, and + `String` scalars. graphql-js 14.5+ (bundled since `@vtex/api@6`) made variable + coercion strict, breaking apps that migrated from `node@4.x` (`@vtex/api@^3`, + graphql 0.13). Incoming variables are now pre-coerced before `execute()` so that + string numbers map to `Int`/`Float` and primitive values map to `String`, + matching the pre-14.5 behavior. Disable with + `VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION=true`. ## [7.3.1] - 2026-01-06 diff --git a/src/service/worker/runtime/graphql/middlewares/run.ts b/src/service/worker/runtime/graphql/middlewares/run.ts index bdca3251c..75e4b06ea 100644 --- a/src/service/worker/runtime/graphql/middlewares/run.ts +++ b/src/service/worker/runtime/graphql/middlewares/run.ts @@ -1,6 +1,7 @@ import { execute } from 'graphql' import { ExecutableSchema, GraphQLServiceContext } from '../typings' +import { coerceVariableValuesLenient } from '../utils/coerceVariablesLenient' export const run = (executableSchema: ExecutableSchema) => async function runHttpQuery(ctx: GraphQLServiceContext, next: () => Promise) { @@ -10,6 +11,7 @@ export const run = (executableSchema: ExecutableSchema) => const { document, operationName, variables: variableValues } = query! const schema = executableSchema.schema + const coercedVariables = coerceVariableValuesLenient(schema, document, variableValues, operationName) const response = await execute({ contextValue: ctx, document, @@ -17,7 +19,7 @@ export const run = (executableSchema: ExecutableSchema) => operationName, rootValue: null, schema, - variableValues, + variableValues: coercedVariables, }) ctx.graphql.graphqlResponse = response diff --git a/src/service/worker/runtime/graphql/utils/coerceVariablesLenient.test.ts b/src/service/worker/runtime/graphql/utils/coerceVariablesLenient.test.ts new file mode 100644 index 000000000..18e50e018 --- /dev/null +++ b/src/service/worker/runtime/graphql/utils/coerceVariablesLenient.test.ts @@ -0,0 +1,409 @@ +import { buildSchema, execute, parse } from 'graphql' + +import { coerceVariableValuesLenient } from './coerceVariablesLenient' + +const SHIPPING_SCHEMA = buildSchema(` + scalar IOUpload + + input ShippingItem { + id: String + quantity: String + seller: String + } + + type LogisticsInfo { + itemIndex: Int + } + + type Shipping { + logisticsInfo: [LogisticsInfo] + } + + type Category { + id: Int + name: String + } + + type Document { + id: String + } + + enum SortOrder { + ASC + DESC + } + + input CustomDoc { + fields: [DocField] + } + + input DocField { + key: String + value: String + } + + type Query { + category(id: Int): Category + categories(treeLevel: Int): [Category] + shipping( + items: [ShippingItem] + postalCode: String + country: String + geoCoordinates: [String] + ): Shipping + documents(acronym: String, page: Int, pageSize: Int): [Document] + enumQuery(order: SortOrder): String + listOfNonNullInts(values: [Int!]!): String + nestedInput(doc: CustomDoc): String + } +`) + +const variablesFor = (query: string, vars: Record, operationName?: string) => + coerceVariableValuesLenient(SHIPPING_SCHEMA, parse(query), vars, operationName) + +describe('coerceVariableValuesLenient', () => { + afterEach(() => { + delete process.env.VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION + }) + + describe('production error patterns', () => { + it('coerces $id: Int with string value (category pattern)', () => { + const result = variablesFor( + `query CategoryChildren($categoryId: Int) { category(id: $categoryId) { id name } }`, + { categoryId: '20' }, + 'CategoryChildren' + ) + expect(result).toEqual({ categoryId: 20 }) + }) + + it('coerces $treeLevel: Int with string value', () => { + const result = variablesFor( + `query categories($treeLevel: Int) { categories(treeLevel: $treeLevel) { id } }`, + { treeLevel: '3' } + ) + expect(result).toEqual({ treeLevel: 3 }) + }) + + it('coerces ShippingItem.quantity (String) when sent as Int', () => { + const result = variablesFor( + `query getShippingEstimates($items: [ShippingItem], $country: String) { + shipping(items: $items, country: $country) { logisticsInfo { itemIndex } } + }`, + { country: 'BRA', items: [{ id: '19361', quantity: 1, seller: '1' }] } + ) + expect(result).toEqual({ + country: 'BRA', + items: [{ id: '19361', quantity: '1', seller: '1' }], + }) + }) + + it('coerces ShippingItem.seller (String) when sent as Int', () => { + const result = variablesFor( + `query($items: [ShippingItem]) { shipping(items: $items) { logisticsInfo { itemIndex } } }`, + { items: [{ id: '1', quantity: '1', seller: 1 }] } + ) + expect(result).toEqual({ items: [{ id: '1', quantity: '1', seller: '1' }] }) + }) + + it('coerces ShippingItem when sent as a single object instead of a list', () => { + const result = variablesFor( + `query getShippingInfo($shippingItems: [ShippingItem]) { + shipping(items: $shippingItems) { logisticsInfo { itemIndex } } + }`, + { shippingItems: { id: '300', seller: '1', quantity: 1 } } + ) + expect(result).toEqual({ shippingItems: { id: '300', seller: '1', quantity: '1' } }) + }) + + it('coerces geoCoordinates: [String] sent as [Float]', () => { + const result = variablesFor( + `query($items: [ShippingItem], $geo: [String], $country: String) { + shipping(items: $items, geoCoordinates: $geo, country: $country) { logisticsInfo { itemIndex } } + }`, + { items: [{ id: '1', quantity: '1', seller: '1' }], geo: [12.4964, 41.8919], country: 'ITA' } + ) + expect(result).toEqual({ + items: [{ id: '1', quantity: '1', seller: '1' }], + geo: ['12.4964', '41.8919'], + country: 'ITA', + }) + }) + + it('coerces postalCode: String when sent as Int', () => { + const result = variablesFor( + `query($items: [ShippingItem], $postalCode: String, $country: String) { + shipping(items: $items, postalCode: $postalCode, country: $country) { logisticsInfo { itemIndex } } + }`, + { items: [{ id: '1', quantity: '1', seller: '1' }], postalCode: 1000, country: 'ARG' } + ) + expect(result).toEqual({ + items: [{ id: '1', quantity: '1', seller: '1' }], + postalCode: '1000', + country: 'ARG', + }) + }) + + it('coerces documents pageSize sent as String', () => { + const result = variablesFor( + `query($acronym: String, $page: Int, $pageSize: Int) { + documents(acronym: $acronym, page: $page, pageSize: $pageSize) { id } + }`, + { acronym: 'NL', page: 1, pageSize: '2' } + ) + expect(result).toEqual({ acronym: 'NL', page: 1, pageSize: 2 }) + }) + }) + + describe('values already correctly typed (no-op)', () => { + it('leaves Int as Int', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: 20 }) + expect(result).toEqual({ id: 20 }) + }) + + it('leaves String as String', () => { + const result = variablesFor( + `query($postalCode: String) { shipping(postalCode: $postalCode) { logisticsInfo { itemIndex } } }`, + { postalCode: '00000' } + ) + expect(result).toEqual({ postalCode: '00000' }) + }) + + it('leaves null/undefined alone', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { + id: null, + }) + expect(result).toEqual({ id: null }) + }) + + it('does not touch enum values', () => { + const result = variablesFor(`query($order: SortOrder) { enumQuery(order: $order) }`, { + order: 'ASC', + }) + expect(result).toEqual({ order: 'ASC' }) + }) + + it('does not coerce custom scalars', () => { + const result = variablesFor(`query($file: IOUpload) { enumQuery(order: ASC) }`, { + file: { name: 'whatever' }, + }) + expect(result).toEqual({ file: { name: 'whatever' } }) + }) + + it('returns the same object shape (does not mutate input)', () => { + const input = { categoryId: '20' } + const result = variablesFor( + `query CategoryChildren($categoryId: Int) { category(id: $categoryId) { id } }`, + input, + 'CategoryChildren' + ) + expect(input).toEqual({ categoryId: '20' }) // original unchanged + expect(result).toEqual({ categoryId: 20 }) + }) + }) + + describe('values that should still raise the standard graphql-js error', () => { + it('does not coerce non-numeric strings to Int', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: 'abc' }) + expect(result).toEqual({ id: 'abc' }) + }) + + it('does not coerce empty string to Int', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: '' }) + expect(result).toEqual({ id: '' }) + }) + + it('does not coerce float string to Int (would lose precision)', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: '20.5' }) + expect(result).toEqual({ id: '20.5' }) + }) + + it('does not coerce arrays to scalar', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: [1, 2] }) + expect(result).toEqual({ id: [1, 2] }) + }) + + it('does not coerce objects to String', () => { + const result = variablesFor( + `query($postalCode: String) { shipping(postalCode: $postalCode) { logisticsInfo { itemIndex } } }`, + { postalCode: { foo: 'bar' } } + ) + expect(result).toEqual({ postalCode: { foo: 'bar' } }) + }) + + it('does not coerce out-of-range ints', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { + id: '99999999999999999', + }) + expect(result).toEqual({ id: '99999999999999999' }) + }) + }) + + describe('type wrappers', () => { + it('coerces inside [Int!]!', () => { + const result = variablesFor(`query($v: [Int!]!) { listOfNonNullInts(values: $v) }`, { + v: ['1', '2', '3'], + }) + expect(result).toEqual({ v: [1, 2, 3] }) + }) + + it('coerces inside nested input objects (CustomDoc.fields[].value)', () => { + const result = variablesFor( + `query($doc: CustomDoc) { nestedInput(doc: $doc) }`, + { doc: { fields: [{ key: 'name', value: 42 }, { key: 'age', value: '13' }] } } + ) + expect(result).toEqual({ + doc: { fields: [{ key: 'name', value: '42' }, { key: 'age', value: '13' }] }, + }) + }) + + it('preserves unknown fields in input objects', () => { + const result = variablesFor( + `query($items: [ShippingItem]) { shipping(items: $items) { logisticsInfo { itemIndex } } }`, + { items: [{ id: '1', quantity: 1, extra: 'preserved' }] as any } + ) + expect(result).toEqual({ items: [{ id: '1', quantity: '1', extra: 'preserved' }] }) + }) + }) + + describe('boolean and edge conversions (graphql 0.13 parity)', () => { + it('coerces true -> 1 for Int (matches Number(true))', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: true }) + expect(result).toEqual({ id: 1 }) + }) + + it('coerces number -> string for String', () => { + const result = variablesFor( + `query($postalCode: String) { shipping(postalCode: $postalCode) { logisticsInfo { itemIndex } } }`, + { postalCode: 1000 } + ) + expect(result).toEqual({ postalCode: '1000' }) + }) + + it('coerces boolean -> string for String', () => { + const result = variablesFor( + `query($postalCode: String) { shipping(postalCode: $postalCode) { logisticsInfo { itemIndex } } }`, + { postalCode: false } + ) + expect(result).toEqual({ postalCode: 'false' }) + }) + }) + + describe('operation selection', () => { + it('uses operationName to disambiguate multiple operations', () => { + const doc = ` + query A($id: Int) { category(id: $id) { id } } + query B($name: String) { shipping(postalCode: $name) { logisticsInfo { itemIndex } } } + ` + const result = variablesFor(doc, { id: '5', name: 9 }, 'B') + // Coerces only $name (used by B); $id is unrelated to B's variable defs + expect(result).toEqual({ id: '5', name: '9' }) + }) + + it('skips coercion when multiple operations and no operationName', () => { + const doc = ` + query A($id: Int) { category(id: $id) { id } } + query B($id: String) { shipping(postalCode: $id) { logisticsInfo { itemIndex } } } + ` + const result = variablesFor(doc, { id: '5' }) + expect(result).toEqual({ id: '5' }) + }) + + it('uses the only operation when no operationName given', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: '5' }) + expect(result).toEqual({ id: 5 }) + }) + }) + + describe('feature flag', () => { + it('does nothing when VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION=true', () => { + process.env.VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION = 'true' + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: '20' }) + expect(result).toEqual({ id: '20' }) + }) + + it('runs normally when env var is unset', () => { + delete process.env.VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: '20' }) + expect(result).toEqual({ id: 20 }) + }) + + it('runs normally when env var has any other value', () => { + process.env.VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION = 'false' + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { id: '20' }) + expect(result).toEqual({ id: 20 }) + }) + }) + + describe('safety', () => { + it('returns input unchanged when there are no variables', () => { + const doc = parse(`{ category(id: 1) { id } }`) + expect(coerceVariableValuesLenient(SHIPPING_SCHEMA, doc, undefined)).toBeUndefined() + expect(coerceVariableValuesLenient(SHIPPING_SCHEMA, doc, {})).toEqual({}) + }) + + it('returns input unchanged for variables not declared in the operation', () => { + const result = variablesFor(`query($id: Int) { category(id: $id) { id } }`, { + id: '20', + unknown: 'kept', + }) + expect(result).toEqual({ id: 20, unknown: 'kept' }) + }) + + it('falls back to original variables on internal failure', () => { + // Empty/invalid document — findOperation returns undefined, no coercion + const fakeDoc = { kind: 'Document', definitions: [] } as any + const result = coerceVariableValuesLenient(SHIPPING_SCHEMA, fakeDoc, { x: '1' }) + expect(result).toEqual({ x: '1' }) + }) + }) + + describe('end-to-end with execute()', () => { + it('lets a query that previously failed now succeed', async () => { + const schema = buildSchema(` + type Query { + category(id: Int): String + } + `) + // Manual root resolver + const rootValue = { category: ({ id }: { id: number }) => `cat-${id}` } + const document = parse(`query CategoryChildren($categoryId: Int) { + category(id: $categoryId) + }`) + const variables = coerceVariableValuesLenient( + schema, + document, + { categoryId: '20' }, + 'CategoryChildren' + ) + const response = await execute({ + schema, + document, + rootValue, + operationName: 'CategoryChildren', + variableValues: variables, + }) + expect(response.errors).toBeUndefined() + expect(response.data).toEqual({ category: 'cat-20' }) + }) + + it('without coercion, the same query produces the production error', async () => { + const schema = buildSchema(` + type Query { + category(id: Int): String + } + `) + const document = parse(`query CategoryChildren($categoryId: Int) { + category(id: $categoryId) + }`) + const response = await execute({ + schema, + document, + rootValue: { category: () => 'should not run' }, + operationName: 'CategoryChildren', + variableValues: { categoryId: '20' }, + }) + expect(response.errors).toBeDefined() + expect(response.errors![0].message).toContain('Int cannot represent non-integer value') + }) + }) +}) diff --git a/src/service/worker/runtime/graphql/utils/coerceVariablesLenient.ts b/src/service/worker/runtime/graphql/utils/coerceVariablesLenient.ts new file mode 100644 index 000000000..bd171b968 --- /dev/null +++ b/src/service/worker/runtime/graphql/utils/coerceVariablesLenient.ts @@ -0,0 +1,254 @@ +/** + * Lenient variable coercion that mimics the behavior of graphql-js < 14.5. + * + * graphql-js 14.5+ made the built-in `Int`, `Float`, and `String` scalars strict: + * `Int.parseValue` rejects strings, `Float.parseValue` rejects strings, and + * `String.parseValue` rejects numbers. Before 14.5 these scalars used `Number(value)` + * and `String(value)` and accepted these conversions silently. + * + * Many existing apps (and their clients) rely on the lenient behavior — for example, + * sending `{ "id": "20" }` for a `$id: Int` variable, or `{ "quantity": 1 }` for an + * input field declared as `String`. After upgrading from `@vtex/api@^3.x` (which + * bundled `graphql@^0.13.2`) to `@vtex/api@^7.x` (which bundles `graphql@^14.5.8`) + * these requests fail with `"Int cannot represent non-integer value: \"20\""` and + * similar errors during `coerceVariableValues`. + * + * This module pre-coerces the incoming `variableValues` object to the types declared + * in the operation's `VariableDefinition`s, replicating the lenient conversions that + * the old runtime did. It only touches values that are unambiguously convertible: + * + * - `Int` ← string of a finite integer, boolean + * - `Float` ← string of a finite number, boolean + * - `String` ← number, boolean + * - `ID` ← number (graphql 14.5 ID is already lenient for strings) + * - `Boolean` ← left alone (no historical lenient conversion) + * + * Anything that doesn't match one of those patterns is left unchanged so that the + * standard `coerceVariableValues` of graphql-js still produces its native error. + * + * The traversal walks `NonNull`, `List`, and `InputObject` types so nested fields + * (e.g. `[ShippingItem!]!.quantity: String`) are coerced too. + * + * Custom scalars are NOT touched — their `parseValue` is whatever the app declared. + * + * Disable with the environment variable `VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION=true` + * to fall back to strict graphql-js behavior. + */ + +import { + DocumentNode, + GraphQLFloat, + GraphQLID, + GraphQLInputObjectType, + GraphQLInputType, + GraphQLInt, + GraphQLScalarType, + GraphQLSchema, + GraphQLString, + isInputObjectType, + isListType, + isNonNullType, + isScalarType, + OperationDefinitionNode, + typeFromAST, +} from 'graphql' + +const MAX_INT = 2147483647 +const MIN_INT = -2147483648 + +const isPlainObject = (value: any): value is Record => + value !== null && typeof value === 'object' && !Array.isArray(value) + +/** + * Coerce a single leaf value against a built-in scalar, replicating the lenient + * conversions of graphql-js 0.13. Returns the coerced value, or the original + * value when no safe conversion applies. + */ +const coerceLeafLenient = (value: any, scalar: GraphQLScalarType): any => { + if (value === null || value === undefined) { + return value + } + + if (scalar === GraphQLInt) { + if (typeof value === 'number') { + return value + } + if (typeof value === 'boolean') { + return value ? 1 : 0 + } + if (typeof value === 'string' && value !== '') { + const num = Number(value) + if (Number.isInteger(num) && num <= MAX_INT && num >= MIN_INT) { + return num + } + } + return value + } + + if (scalar === GraphQLFloat) { + if (typeof value === 'number') { + return value + } + if (typeof value === 'boolean') { + return value ? 1 : 0 + } + if (typeof value === 'string' && value !== '') { + const num = Number(value) + if (Number.isFinite(num)) { + return num + } + } + return value + } + + if (scalar === GraphQLString) { + if (typeof value === 'string') { + return value + } + if (typeof value === 'number' || typeof value === 'boolean') { + return String(value) + } + return value + } + + if (scalar === GraphQLID) { + if (typeof value === 'number' && Number.isFinite(value)) { + return String(value) + } + return value + } + + return value +} + +/** + * Recursively coerce a value against a GraphQL input type. Walks NonNull/List + * wrappers and InputObject fields. Enum and custom scalar leaves are returned + * unchanged. + */ +const coerceValueAgainstType = (value: any, type: GraphQLInputType): any => { + if (value === null || value === undefined) { + return value + } + + if (isNonNullType(type)) { + return coerceValueAgainstType(value, type.ofType as GraphQLInputType) + } + + if (isListType(type)) { + const itemType = type.ofType as GraphQLInputType + if (Array.isArray(value)) { + return value.map(item => coerceValueAgainstType(item, itemType)) + } + // graphql-js coerces a single value to a one-element list; we mirror that + // by coercing the lone value but leave the wrapping shape to graphql-js. + return coerceValueAgainstType(value, itemType) + } + + if (isInputObjectType(type)) { + if (!isPlainObject(value)) { + return value + } + const fields = (type as GraphQLInputObjectType).getFields() + const out: Record = {} + for (const key of Object.keys(value)) { + const fieldDef = fields[key] + if (fieldDef) { + out[key] = coerceValueAgainstType(value[key], fieldDef.type as GraphQLInputType) + } else { + // Unknown field: keep as-is so graphql-js produces its own error. + out[key] = value[key] + } + } + return out + } + + if (isScalarType(type)) { + return coerceLeafLenient(value, type) + } + + // Enum or anything else: leave alone, graphql-js will validate. + return value +} + +const findOperation = ( + document: DocumentNode, + operationName?: string +): OperationDefinitionNode | undefined => { + let firstOperation: OperationDefinitionNode | undefined + for (const def of document.definitions) { + if (def.kind !== 'OperationDefinition') { + continue + } + if (operationName) { + if (def.name && def.name.value === operationName) { + return def + } + } else { + if (firstOperation) { + // Multiple operations and no operationName — graphql-js will reject; + // skip lenient coercion in this ambiguous case. + return undefined + } + firstOperation = def + } + } + return firstOperation +} + +const isDisabledByEnv = (): boolean => + process.env.VTEX_API_DISABLE_LENIENT_VARIABLE_COERCION === 'true' + +/** + * Pre-process variable values for an incoming GraphQL operation, applying the + * pre-14.5 lenient scalar coercion rules. Returns a new object so the caller's + * input is not mutated. + * + * On any unexpected error (malformed document, type resolution failure, etc.) + * returns the original `variableValues` unchanged so graphql-js can produce its + * own error. + */ +export const coerceVariableValuesLenient = ( + schema: GraphQLSchema, + document: DocumentNode, + variableValues: Record | undefined, + operationName?: string +): Record | undefined => { + if (!variableValues || isDisabledByEnv()) { + return variableValues + } + + try { + const operation = findOperation(document, operationName) + if (!operation || !operation.variableDefinitions || operation.variableDefinitions.length === 0) { + return variableValues + } + + const coerced: Record = { ...variableValues } + for (const varDef of operation.variableDefinitions) { + const name = varDef.variable.name.value + if (!(name in coerced)) { + continue + } + const type = typeFromAST(schema, varDef.type as any) as GraphQLInputType | undefined + if (!type) { + continue + } + coerced[name] = coerceValueAgainstType(coerced[name], type) + } + return coerced + } catch { + return variableValues + } +} + +// Re-exported for tests so we don't have to roundtrip through a document/schema +// to exercise the leaf logic. +export const testing = { + GraphQLFloat, + GraphQLID, + GraphQLInt, + GraphQLString, + coerceLeafLenient, + coerceValueAgainstType, +}