From d1209ae5dcabdfdc0ed2e1daf2f615b9cc7b2a76 Mon Sep 17 00:00:00 2001 From: Ian Hoffman Date: Wed, 8 Mar 2023 17:12:52 -0500 Subject: [PATCH 1/3] Fix codegen for empty shapes We were returning `dict[]` from functions typed as returning empty shapes. Sort of surprising this only just came up, but here's the fix. --- src/Codegen/Constraints/ObjectBuilder.php | 2 +- tests/ObjectSchemaValidatorTest.php | 25 ++++++++ .../codegen/ObjectSchemaValidator.php | 57 ++++++++++++++++++- tests/examples/object-schema.json | 6 ++ 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/src/Codegen/Constraints/ObjectBuilder.php b/src/Codegen/Constraints/ObjectBuilder.php index d41ae3c..3c26195 100644 --- a/src/Codegen/Constraints/ObjectBuilder.php +++ b/src/Codegen/Constraints/ObjectBuilder.php @@ -153,7 +153,7 @@ protected function getCheckMethodCode( $hb->addInlineComment('Hack to prevent us from having to change the params names when we are not using them.'); $hb->addAssignment('$_', '$input', HackBuilderValues::literal()); $hb->addAssignment('$_', '$pointer', HackBuilderValues::literal()); - $hb->addReturn('dict[]', HackBuilderValues::literal()); + $hb->addReturn('shape()', HackBuilderValues::literal()); return $hb; } diff --git a/tests/ObjectSchemaValidatorTest.php b/tests/ObjectSchemaValidatorTest.php index 5791f6d..dcf4f29 100644 --- a/tests/ObjectSchemaValidatorTest.php +++ b/tests/ObjectSchemaValidatorTest.php @@ -692,4 +692,29 @@ public function testInvalidMinPropertiesWithNoAdditionalProperties(): void { expect($constraint['got'] ?? null)->toEqual('a'); } + public function testEmptyClosedShape(): void { + $validator = new ObjectSchemaValidator(dict[ + 'empty_closed_shape' => dict[] + ]); + $validator->validate(); + expect($validator->isValid())->toBeTrue(); + + $validator = new ObjectSchemaValidator(dict[ + 'empty_closed_shape' => dict['foo' => 'bar'] + ]); + $validator->validate(); + expect($validator->isValid())->toBeFalse(); + + $errors = $validator->getErrors(); + expect(C\count($errors))->toEqual(1); + + $error = C\firstx($errors); + expect($error['code'])->toEqual(FieldErrorCode::FAILED_CONSTRAINT); + expect($error['message'])->toEqual('invalid additional property: foo'); + + $constraint = Shapes::at($error, 'constraint'); + expect($constraint['type'])->toEqual(FieldErrorConstraint::ADDITIONAL_PROPERTIES); + expect($constraint['got'] ?? null)->toEqual('foo'); + } + } diff --git a/tests/examples/codegen/ObjectSchemaValidator.php b/tests/examples/codegen/ObjectSchemaValidator.php index b7237da..870f24a 100644 --- a/tests/examples/codegen/ObjectSchemaValidator.php +++ b/tests/examples/codegen/ObjectSchemaValidator.php @@ -5,7 +5,7 @@ * To re-generate this file run `make test` * * - * @generated SignedSource<<7fa7d1f5ae7e84bf34ab173568728f05>> + * @generated SignedSource<<5d8158e850af0f6d550037bc4003da67>> */ namespace Slack\Hack\JsonSchema\Tests\Generated; use namespace Slack\Hack\JsonSchema; @@ -97,6 +97,9 @@ type TObjectSchemaValidatorPropertiesInvalidMinPropertiesWithNoAdditionalProperties = dict; +type TObjectSchemaValidatorPropertiesEmptyClosedShape = shape( +); + type TObjectSchemaValidator = shape( ?'only_additional_properties' => TObjectSchemaValidatorPropertiesOnlyAdditionalProperties, ?'only_no_additional_properties' => TObjectSchemaValidatorPropertiesOnlyNoAdditionalProperties, @@ -119,6 +122,7 @@ ?'only_max_properties' => TObjectSchemaValidatorPropertiesOnlyMaxProperties, ?'min_and_max_properties' => TObjectSchemaValidatorPropertiesMinAndMaxProperties, ?'invalid_min_properties_with_no_additional_properties' => TObjectSchemaValidatorPropertiesInvalidMinPropertiesWithNoAdditionalProperties, + ?'empty_closed_shape' => TObjectSchemaValidatorPropertiesEmptyClosedShape, ... ); @@ -1648,6 +1652,46 @@ public static function check( } } +final class ObjectSchemaValidatorPropertiesEmptyClosedShape { + + private static bool $coerce = false; + private static keyset $properties = keyset[ + ]; + + public static function check( + mixed $input, + string $pointer, + ): TObjectSchemaValidatorPropertiesEmptyClosedShape { + $typed = Constraints\ObjectConstraint::check($input, $pointer, self::$coerce); + + $errors = vec[]; + $output = shape(); + + /*HHAST_IGNORE_ERROR[UnusedVariable] Some loops generated with this statement do not use their $value*/ + foreach ($typed as $key => $value) { + if (\HH\Lib\C\contains_key(self::$properties, $key)) { + continue; + } + + $errors[] = shape( + 'code' => JsonSchema\FieldErrorCode::FAILED_CONSTRAINT, + 'message' => "invalid additional property: {$key}", + 'constraint' => shape( + 'type' => JsonSchema\FieldErrorConstraint::ADDITIONAL_PROPERTIES, + 'got' => $key, + ), + ); + } + + if (\HH\Lib\C\count($errors)) { + throw new JsonSchema\InvalidFieldException($pointer, $errors); + } + + /* HH_IGNORE_ERROR[4163] */ + return $output; + } +} + final class ObjectSchemaValidator extends JsonSchema\BaseValidator { @@ -1899,6 +1943,17 @@ public static function check( } } + if (\HH\Lib\C\contains_key($typed, 'empty_closed_shape')) { + try { + $output['empty_closed_shape'] = ObjectSchemaValidatorPropertiesEmptyClosedShape::check( + $typed['empty_closed_shape'], + JsonSchema\get_pointer($pointer, 'empty_closed_shape'), + ); + } catch (JsonSchema\InvalidFieldException $e) { + $errors = \HH\Lib\Vec\concat($errors, $e->errors); + } + } + if (\HH\Lib\C\count($errors)) { throw new JsonSchema\InvalidFieldException($pointer, $errors); } diff --git a/tests/examples/object-schema.json b/tests/examples/object-schema.json index 543f90e..db8ab3d 100644 --- a/tests/examples/object-schema.json +++ b/tests/examples/object-schema.json @@ -176,6 +176,12 @@ "type": "object", "minProperties": 1, "additionalProperties": false + }, + "empty_closed_shape": { + "type": "object", + "additionalProperties": false, + "discardAdditionalProperties": false, + "properties": {} } } } From 8b894cd55377c736f8d61a2aace46cc7b5bdc592 Mon Sep 17 00:00:00 2001 From: Ian Hoffman Date: Thu, 9 Mar 2023 10:09:29 -0500 Subject: [PATCH 2/3] use discardAdditionalProperties: true so we actually test the right thing --- tests/ObjectSchemaValidatorTest.php | 16 +++------ .../codegen/ObjectSchemaValidator.php | 34 ++++--------------- tests/examples/object-schema.json | 2 +- 3 files changed, 11 insertions(+), 41 deletions(-) diff --git a/tests/ObjectSchemaValidatorTest.php b/tests/ObjectSchemaValidatorTest.php index dcf4f29..fca7bd2 100644 --- a/tests/ObjectSchemaValidatorTest.php +++ b/tests/ObjectSchemaValidatorTest.php @@ -698,23 +698,15 @@ public function testEmptyClosedShape(): void { ]); $validator->validate(); expect($validator->isValid())->toBeTrue(); + expect($validator->getValidatedInput())->toEqual(shape()); + // Additional properties are discarded $validator = new ObjectSchemaValidator(dict[ 'empty_closed_shape' => dict['foo' => 'bar'] ]); $validator->validate(); - expect($validator->isValid())->toBeFalse(); - - $errors = $validator->getErrors(); - expect(C\count($errors))->toEqual(1); - - $error = C\firstx($errors); - expect($error['code'])->toEqual(FieldErrorCode::FAILED_CONSTRAINT); - expect($error['message'])->toEqual('invalid additional property: foo'); - - $constraint = Shapes::at($error, 'constraint'); - expect($constraint['type'])->toEqual(FieldErrorConstraint::ADDITIONAL_PROPERTIES); - expect($constraint['got'] ?? null)->toEqual('foo'); + expect($validator->isValid())->toBeTrue(); + expect($validator->getValidatedInput())->toEqual(shape()); } } diff --git a/tests/examples/codegen/ObjectSchemaValidator.php b/tests/examples/codegen/ObjectSchemaValidator.php index 870f24a..3ec1e0b 100644 --- a/tests/examples/codegen/ObjectSchemaValidator.php +++ b/tests/examples/codegen/ObjectSchemaValidator.php @@ -5,7 +5,7 @@ * To re-generate this file run `make test` * * - * @generated SignedSource<<5d8158e850af0f6d550037bc4003da67>> + * @generated SignedSource<<39430f90b530b5635008632715853853>> */ namespace Slack\Hack\JsonSchema\Tests\Generated; use namespace Slack\Hack\JsonSchema; @@ -1662,33 +1662,11 @@ public static function check( mixed $input, string $pointer, ): TObjectSchemaValidatorPropertiesEmptyClosedShape { - $typed = Constraints\ObjectConstraint::check($input, $pointer, self::$coerce); - - $errors = vec[]; - $output = shape(); - - /*HHAST_IGNORE_ERROR[UnusedVariable] Some loops generated with this statement do not use their $value*/ - foreach ($typed as $key => $value) { - if (\HH\Lib\C\contains_key(self::$properties, $key)) { - continue; - } - - $errors[] = shape( - 'code' => JsonSchema\FieldErrorCode::FAILED_CONSTRAINT, - 'message' => "invalid additional property: {$key}", - 'constraint' => shape( - 'type' => JsonSchema\FieldErrorConstraint::ADDITIONAL_PROPERTIES, - 'got' => $key, - ), - ); - } - - if (\HH\Lib\C\count($errors)) { - throw new JsonSchema\InvalidFieldException($pointer, $errors); - } - - /* HH_IGNORE_ERROR[4163] */ - return $output; + // Hack to prevent us from having to change the params names when we are not + // using them. + $_ = $input; + $_ = $pointer; + return shape(); } } diff --git a/tests/examples/object-schema.json b/tests/examples/object-schema.json index db8ab3d..d671083 100644 --- a/tests/examples/object-schema.json +++ b/tests/examples/object-schema.json @@ -180,7 +180,7 @@ "empty_closed_shape": { "type": "object", "additionalProperties": false, - "discardAdditionalProperties": false, + "discardAdditionalProperties": true, "properties": {} } } From 54a6298ab4346a44e5519d80b7665aea16d2291e Mon Sep 17 00:00:00 2001 From: Ian Hoffman Date: Thu, 9 Mar 2023 18:00:27 -0500 Subject: [PATCH 3/3] test fix --- tests/ObjectSchemaValidatorTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ObjectSchemaValidatorTest.php b/tests/ObjectSchemaValidatorTest.php index fca7bd2..635e40b 100644 --- a/tests/ObjectSchemaValidatorTest.php +++ b/tests/ObjectSchemaValidatorTest.php @@ -698,7 +698,7 @@ public function testEmptyClosedShape(): void { ]); $validator->validate(); expect($validator->isValid())->toBeTrue(); - expect($validator->getValidatedInput())->toEqual(shape()); + expect($validator->getValidatedInput())->toEqual(shape('empty_closed_shape' => shape())); // Additional properties are discarded $validator = new ObjectSchemaValidator(dict[ @@ -706,7 +706,7 @@ public function testEmptyClosedShape(): void { ]); $validator->validate(); expect($validator->isValid())->toBeTrue(); - expect($validator->getValidatedInput())->toEqual(shape()); + expect($validator->getValidatedInput())->toEqual(shape('empty_closed_shape' => shape())); } }