Skip to content

Commit 63288b2

Browse files
Prioritize count error over null error in @OneOf coercion (#1891)
* Prioritize count error over null error in @OneOf coercion When a client sends multiple fields for a @OneOf input (e.g. one non-null and one null), the "must specify exactly one field" error is more actionable than "field X must be non-null", which misleadingly suggests providing a value for the null field. This aligns Value::coerceInputValue with the existing behavior in OneOfInputObjectsRule, which already checks count before null. * Autofix --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 parent ca9a962 commit 63288b2

2 files changed

Lines changed: 122 additions & 21 deletions

File tree

src/Utils/Value.php

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -182,33 +182,27 @@ public static function coerceInputValue($value, InputType $type, ?array $path =
182182

183183
// Validate OneOf constraints if this is a OneOf input type
184184
if ($type->isOneOf()) {
185-
$providedFieldCount = 0;
185+
$providedFieldCount = count($coercedValue);
186186
$nullFieldName = null;
187187

188-
foreach ($coercedValue as $fieldName => $fieldValue) {
189-
if ($fieldValue !== null) {
190-
++$providedFieldCount;
191-
} else {
192-
$nullFieldName = $fieldName;
193-
}
194-
}
195-
196-
// Check for null field values first (takes precedence)
197-
if ($nullFieldName !== null) {
198-
$errors = self::add(
199-
$errors,
200-
CoercionError::make("OneOf input object \"{$type->name}\" field \"{$nullFieldName}\" must be non-null.", $path, $value)
201-
);
202-
} elseif ($providedFieldCount === 0) {
203-
$errors = self::add(
204-
$errors,
205-
CoercionError::make("OneOf input object \"{$type->name}\" must specify exactly one field.", $path, $value)
206-
);
207-
} elseif ($providedFieldCount > 1) {
188+
if ($providedFieldCount !== 1) {
208189
$errors = self::add(
209190
$errors,
210191
CoercionError::make("OneOf input object \"{$type->name}\" must specify exactly one field.", $path, $value)
211192
);
193+
} else {
194+
foreach ($coercedValue as $fieldName => $fieldValue) {
195+
if ($fieldValue === null) {
196+
$nullFieldName = $fieldName;
197+
}
198+
}
199+
200+
if ($nullFieldName !== null) {
201+
$errors = self::add(
202+
$errors,
203+
CoercionError::make("OneOf input object \"{$type->name}\" field \"{$nullFieldName}\" must be non-null.", $path, $value)
204+
);
205+
}
212206
}
213207
}
214208

tests/Type/OneOfInputObjectTest.php

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,4 +250,111 @@ public function testOneOfCoercionValidation(): void
250250
self::assertCount(1, $nullFieldResult['errors']);
251251
self::assertEquals('OneOf input object "OneOfInput" field "stringField" must be non-null.', $nullFieldResult['errors'][0]->getMessage());
252252
}
253+
254+
/**
255+
* When a nullable @oneOf field is passed as null via variables,
256+
* no @oneOf validation should trigger.
257+
*/
258+
public function testNullableOneOfFieldPassedAsNullVariable(): void
259+
{
260+
$schema = $this->buildOneOfSchema();
261+
262+
$result = GraphQL::executeQuery(
263+
$schema,
264+
'{ test(input: { oneOfField: null, name: "hello" }) }',
265+
);
266+
267+
self::assertEmpty($result->errors, sprintf(
268+
'Expected no errors when passing null for a nullable @oneOf field, got: %s',
269+
implode(', ', array_map(static fn ($e) => $e->getMessage(), $result->errors)),
270+
));
271+
}
272+
273+
/**
274+
* When a nullable @oneOf field is omitted entirely,
275+
* no @oneOf validation should trigger.
276+
*/
277+
public function testNullableOneOfFieldOmittedFromInput(): void
278+
{
279+
$schema = $this->buildOneOfSchema();
280+
281+
$result = GraphQL::executeQuery(
282+
$schema,
283+
'{ test(input: { name: "hello" }) }',
284+
);
285+
286+
self::assertEmpty($result->errors, sprintf(
287+
'Expected no errors when omitting a nullable @oneOf field, got: %s',
288+
implode(', ', array_map(static fn ($e) => $e->getMessage(), $result->errors)),
289+
));
290+
}
291+
292+
/**
293+
* When both @oneOf fields are provided (one non-null, one null), the count
294+
* check should take precedence over the null check, producing the more
295+
* actionable "must specify exactly one field" error.
296+
*/
297+
public function testOneOfCoercionWithMultipleFieldsReportsCountErrorOverNullError(): void
298+
{
299+
$oneOfType = new InputObjectType([
300+
'name' => 'OneOfInput',
301+
'fields' => [
302+
'stringField' => Type::string(),
303+
'intField' => Type::int(),
304+
],
305+
'isOneOf' => true,
306+
]);
307+
308+
$result = Value::coerceInputValue(['stringField' => 'test', 'intField' => null], $oneOfType);
309+
self::assertNotNull($result['errors']);
310+
self::assertCount(1, $result['errors']);
311+
self::assertEquals('OneOf input object "OneOfInput" must specify exactly one field.', $result['errors'][0]->getMessage());
312+
}
313+
314+
/**
315+
* Builds a schema with a regular input type that contains a nullable @oneOf field.
316+
*
317+
* input TestInput {
318+
* oneOfField: OneOfInput
319+
* name: String!
320+
* }
321+
* input OneOfInput @oneOf {
322+
* stringField: String
323+
* intField: Int
324+
* }
325+
*
326+
* @throws \GraphQL\Error\InvariantViolation
327+
*/
328+
private function buildOneOfSchema(): Schema
329+
{
330+
$oneOfInput = new InputObjectType([
331+
'name' => 'OneOfInput',
332+
'isOneOf' => true,
333+
'fields' => [
334+
'stringField' => Type::string(),
335+
'intField' => Type::int(),
336+
],
337+
]);
338+
339+
$testInput = new InputObjectType([
340+
'name' => 'TestInput',
341+
'fields' => [
342+
'oneOfField' => $oneOfInput,
343+
'name' => Type::nonNull(Type::string()),
344+
],
345+
]);
346+
347+
return new Schema([
348+
'query' => new ObjectType([
349+
'name' => 'Query',
350+
'fields' => [
351+
'test' => [
352+
'type' => Type::string(),
353+
'args' => ['input' => Type::nonNull($testInput)],
354+
'resolve' => static fn () => 'test',
355+
],
356+
],
357+
]),
358+
]);
359+
}
253360
}

0 commit comments

Comments
 (0)