Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improvements to type coercion #384

Merged
merged 9 commits into from
Mar 21, 2017
Merged

Improvements to type coercion #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba33726
Improve performance - don't loop over everything if already valid
erayd Mar 13, 2017
7f7a2b1
Don't coerce already-valid types (fixes #379)
erayd Mar 13, 2017
d70c8d6
Add remaining coercion cases & rewrite tests
erayd Mar 13, 2017
a775d4d
Add CHECK_MODE_EARLY_COERCE
erayd Mar 14, 2017
2ea432b
Add multiple-type test that requires coercion
erayd Mar 14, 2017
4ca0541
\JSON_PRETTY_PRINT doesn't exist in PHP-5.3, so work around this
erayd Mar 14, 2017
ab0e8ba
Various PR cleanup stuff
erayd Mar 14, 2017
c8f4b50
Move loop after complex tests definition
erayd Mar 14, 2017
a21741d
Move test #39 to grid #15
erayd Mar 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,19 @@ third argument to `Validator::validate()`, or can be provided as the third argum
| `Constraint::CHECK_MODE_NORMAL` | Validate in 'normal' mode - this is the default |
| `Constraint::CHECK_MODE_TYPE_CAST` | Enable fuzzy type checking for associative arrays and objects |
| `Constraint::CHECK_MODE_COERCE_TYPES` | Convert data types to match the schema where possible |
| `Constraint::CHECK_MODE_EARLY_COERCE` | Apply type coercion as soon as possible |
| `Constraint::CHECK_MODE_APPLY_DEFAULTS` | Apply default values from the schema if not set |
| `Constraint::CHECK_MODE_ONLY_REQUIRED_DEFAULTS` | When applying defaults, only set values that are required |
| `Constraint::CHECK_MODE_EXCEPTIONS` | Throw an exception immediately if validation fails |
| `Constraint::CHECK_MODE_DISABLE_FORMAT` | Do not validate "format" constraints |
| `Constraint::CHECK_MODE_VALIDATE_SCHEMA` | Validate the schema as well as the provided document |

Please note that using `Constraint::CHECK_MODE_COERCE_TYPES` or `Constraint::CHECK_MODE_APPLY_DEFAULTS`
will modify your original data.
Please note that using `CHECK_MODE_COERCE_TYPES` or `CHECK_MODE_APPLY_DEFAULTS` will modify your
original data.

`CHECK_MODE_EARLY_COERCE` has no effect unless used in combination with `CHECK_MODE_COERCE_TYPES`. If
enabled, the validator will use (and coerce) the first compatible type it encounters, even if the
schema defines another type that matches directly and does not require coercion.

## Running the tests

Expand Down
1 change: 1 addition & 0 deletions src/JsonSchema/Constraints/Constraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ abstract class Constraint extends BaseConstraint implements ConstraintInterface
const CHECK_MODE_APPLY_DEFAULTS = 0x00000008;
const CHECK_MODE_EXCEPTIONS = 0x00000010;
const CHECK_MODE_DISABLE_FORMAT = 0x00000020;
const CHECK_MODE_EARLY_COERCE = 0x00000040;
const CHECK_MODE_ONLY_REQUIRED_DEFAULTS = 0x00000080;
const CHECK_MODE_VALIDATE_SCHEMA = 0x00000100;

Expand Down
130 changes: 115 additions & 15 deletions src/JsonSchema/Constraints/TypeConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,24 @@ public function check(&$value = null, $schema = null, JsonPointer $path = null,
{
$type = isset($schema->type) ? $schema->type : null;
$isValid = false;
$coerce = $this->factory->getConfig(self::CHECK_MODE_COERCE_TYPES);
$earlyCoerce = $this->factory->getConfig(self::CHECK_MODE_EARLY_COERCE);
$wording = array();

if (is_array($type)) {
$this->validateTypesArray($value, $type, $wording, $isValid, $path);
$this->validateTypesArray($value, $type, $wording, $isValid, $path, $coerce && $earlyCoerce);
if (!$isValid && $coerce && !$earlyCoerce) {
$this->validateTypesArray($value, $type, $wording, $isValid, $path, true);
}
} elseif (is_object($type)) {
$this->checkUndefined($value, $type, $path);

return;
} else {
$isValid = $this->validateType($value, $type);
$isValid = $this->validateType($value, $type, $coerce && $earlyCoerce);
if (!$isValid && $coerce && !$earlyCoerce) {
$isValid = $this->validateType($value, $type, true);
}
}

if ($isValid === false) {
Expand All @@ -62,8 +70,8 @@ public function check(&$value = null, $schema = null, JsonPointer $path = null,
$wording[] = self::$wording[$type];
}
$this->addError(ConstraintError::TYPE(), $path, array(
'expected' => gettype($value),
'found' => $this->implodeWith($wording, ', ', 'or')
'found' => gettype($value),
'expected' => $this->implodeWith($wording, ', ', 'or')
));
}
}
Expand All @@ -79,9 +87,14 @@ public function check(&$value = null, $schema = null, JsonPointer $path = null,
* @param bool $isValid The current validation value
* @param $path
*/
protected function validateTypesArray(&$value, array $type, &$validTypesWording, &$isValid, $path)
protected function validateTypesArray(&$value, array $type, &$validTypesWording, &$isValid, $path, $coerce = false)
{
foreach ($type as $tp) {
// already valid, so no need to waste cycles looping over everything
if ($isValid) {
return;
}

// $tp can be an object, if it's a schema instead of a simple type, validate it
// with a new type constraint
if (is_object($tp)) {
Expand All @@ -98,7 +111,7 @@ protected function validateTypesArray(&$value, array $type, &$validTypesWording,
$this->validateTypeNameWording($tp);
$validTypesWording[] = self::$wording[$tp];
if (!$isValid) {
$isValid = $this->validateType($value, $tp);
$isValid = $this->validateType($value, $tp, $coerce);
}
}
}
Expand Down Expand Up @@ -157,7 +170,7 @@ protected function validateTypeNameWording($type)
*
* @return bool
*/
protected function validateType(&$value, $type)
protected function validateType(&$value, $type, $coerce = false)
{
//mostly the case for inline schema
if (!$type) {
Expand All @@ -173,11 +186,13 @@ protected function validateType(&$value, $type)
}

if ('array' === $type) {
if ($coerce) {
$value = $this->toArray($value);
Copy link
Collaborator

@shmax shmax Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I notice that ajv only does this conversion if coerceTypes is set to "array". Should this be controlled by another flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point. I'm happy either way - would you like a flag?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly don't know. We can try without for a while, and see if the rationale for having one ever emerges.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion - let's do that then. And if someone asks for it, then we can add it at that point.

I don't supposed you know whether @epoberezkin has stated his rationale for doing it that way in ajv?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think this is the issue that spawned the feature: ajv-validator/ajv#158

They discuss it a bit, then come up with the coerceTypes:'array' idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... will have to think on this a bit. At a minimum I should write some tests to check that post-coercion validation is happening properly.

}

return $this->getTypeCheck()->isArray($value);
}

$coerce = $this->factory->getConfig(Constraint::CHECK_MODE_COERCE_TYPES);

if ('integer' === $type) {
if ($coerce) {
$value = $this->toInteger($value);
Expand All @@ -203,10 +218,18 @@ protected function validateType(&$value, $type)
}

if ('string' === $type) {
if ($coerce) {
$value = $this->toString($value);
}

return is_string($value);
}

if ('null' === $type) {
if ($coerce) {
$value = $this->toNull($value);
}

return is_null($value);
}

Expand All @@ -222,19 +245,21 @@ protected function validateType(&$value, $type)
*/
protected function toBoolean($value)
{
if ($value === 'true') {
if ($value === 1 || $value === 'true') {
return true;
}

if ($value === 'false') {
if (is_null($value) || $value === 0 || $value === 'false') {
return false;
}
if ($this->getTypeCheck()->isArray($value) && count($value) === 1) {
return $this->toBoolean(reset($value));
}

return $value;
}

/**
* Converts a numeric string to a number. For example, "4" becomes 4.
* Converts a value to a number. For example, "4.5" becomes 4.5.
*
* @param mixed $value the value to convert to a number
*
Expand All @@ -245,14 +270,89 @@ protected function toNumber($value)
if (is_numeric($value)) {
return $value + 0; // cast to number
}
if (is_bool($value) || is_null($value)) {
return (int) $value;
}
if ($this->getTypeCheck()->isArray($value) && count($value) === 1) {
return $this->toNumber(reset($value));
}

return $value;
}

/**
* Converts a value to an integer. For example, "4" becomes 4.
*
* @param mixed $value
*
* @return int|mixed
*/
protected function toInteger($value)
{
if (is_numeric($value) && (int) $value == $value) {
return (int) $value; // cast to number
$numberValue = $this->toNumber($value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does the call to toNumber get you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other type coersion cases. It means no copy pasta, because other than integer-ness, the two methods are identical.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything actually functionally different? Because now we're calling is_numeric twice whereas before it was only called once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - the second is_numeric call catches cases that fall through toNumber without being successfully coerced.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what I mean, though--if you put the code back the way it was then there is only one call to is_numeric. So I guess I'm still not clear on what is functionally different about the new code. Are additional values now being coerced to integer (eg. boolean) with this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The new code also coerces from bool, null, and array, via the call to toNumber.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the logic is to apply all the coercion rules for number, and then see if the result can be further coerced to an int. If it is, then return the cast int - if not, return the original value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okie-doke

if (is_numeric($numberValue) && (int) $numberValue == $numberValue) {
return (int) $numberValue; // cast to number
}

return $value;
}

/**
* Converts a value to an array containing that value. For example, [4] becomes 4.
*
* @param mixed $value
*
* @return array|mixed
*/
protected function toArray($value)
{
if (is_scalar($value) || is_null($value)) {
return array($value);
}

return $value;
}

/**
* Convert a value to a string representation of that value. For example, null becomes "".
*
* @param mixed $value
*
* @return string|mixed
*/
protected function toString($value)
{
if (is_numeric($value)) {
return "$value";
}
if ($value === true) {
return 'true';
}
if ($value === false) {
return 'false';
}
if (is_null($value)) {
return '';
}
if ($this->getTypeCheck()->isArray($value) && count($value) === 1) {
return $this->toString(reset($value));
}
}

/**
* Convert a value to a null. For example, 0 becomes null.
*
* @param mixed $value
*
* @return null|mixed
*/
protected function toNull($value)
{
if ($value === 0 || $value === false || $value === '') {
return null;
}
if ($this->getTypeCheck()->isArray($value) && count($value) === 1) {
return $this->toNull(reset($value));
}

return $value;
Expand Down
Loading