Skip to content

Better coercion #336

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

Merged
merged 2 commits into from
Dec 22, 2016
Merged

Better coercion #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1224341
Better coercion
shmax Nov 26, 2016
8885faf
add phpdocs
shmax Dec 12, 2016
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: 2 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,7 @@ $request = (object)[
'refundAmount'=>"17"
];

$factory = new Factory( null, null, Constraint::CHECK_MODE_TYPE_CAST | Constraint::CHECK_MODE_COERCE );

$validator = new Validator($factory);
$validator->check($request, (object) [
$validator->coerce($request, (object) [
"type"=>"object",
"properties"=>(object)[
"processRefund"=>(object)[
Expand All @@ -82,8 +79,6 @@ is_bool($request->processRefund); // true
is_int($request->refundAmount); // true
```

Note that the `CHECK_MODE_COERCE` flag will only take effect when an object is passed into the `check()` method.

### With inline references

```php
Expand Down Expand Up @@ -144,4 +139,4 @@ $jsonValidator->check($jsonToValidateObject, $jsonSchemaObject);
composer test
composer testOnly TestClass
composer testOnly TestClass::testMethod
```
```
49 changes: 32 additions & 17 deletions src/JsonSchema/Constraints/CollectionConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,24 @@
*/
class CollectionConstraint extends Constraint
{

/**
* {@inheritDoc}
*/
public function check($value, $schema = null, JsonPointer $path = null, $i = null)
{
$this->_check($value, $schema, $path, $i);
}

/**
* {@inheritDoc}
*/
public function coerce(&$value, $schema = null, JsonPointer $path = null, $i = null)
{
$this->_check($value, $schema, $path, $i, true);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

you should fix the indentation here. maybe we should create a PR with php-cs-fixer & style-ci

protected function _check(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

mh maybe we can make it even more BC by just providing a CoerceConstraint (which wraps the underlying Constraint) and does all the type checking. so in my educated guess we only pass the coerce flag to the factory so it wraps the wanted constraint with the CoerceConstraint. The Facade-Pattern it is ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

so all other constraints are mostly untouched...
like this:

class CoerceConstraint
{
    public function __construct(Constraint $constraint)
    {
        $this->constraint  = $constraint;
    }

    public function check(...)
    {
        $result = $this->constraint->check(...);
        
       //do the coercion here...
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting idea, but unfortunately any kind of coercion that depends on check is a non-starter because I would have to change the first argument to a reference, which would break bc.

{
// Verify minItems
if (isset($schema->minItems) && count($value) < $schema->minItems) {
Expand All @@ -47,7 +61,7 @@ public function check($value, $schema = null, JsonPointer $path = null, $i = nul

// Verify items
if (isset($schema->items)) {
$this->validateItems($value, $schema, $path, $i);
$this->validateItems($value, $schema, $path, $i, $coerce);
}
}

Expand All @@ -58,8 +72,9 @@ public function check($value, $schema = null, JsonPointer $path = null, $i = nul
* @param \stdClass $schema
* @param JsonPointer|null $path
* @param string $i
* @param boolean $coerce
*/
protected function validateItems($value, $schema = null, JsonPointer $path = null, $i = null)
protected function validateItems(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)
{
if (is_object($schema->items)) {
// just one type definition for the whole array
Expand All @@ -74,27 +89,27 @@ protected function validateItems($value, $schema = null, JsonPointer $path = nul
) {
// performance optimization
$type = $schema->items->type;
$validator = $this->factory->createInstanceFor($type === 'integer' ? 'number' : $type);
$typeValidator = $this->factory->createInstanceFor('type');
$validator = $this->factory->createInstanceFor($type === 'integer' ? 'number' : $type);

foreach ($value as $k => $v) {
$k_path = $this->incrementPath($path, $k);
$k_path = $this->incrementPath($path, $k);
if($coerce) {
$typeValidator->coerce($v, $schema->items, $k_path, $i);
} else {
$typeValidator->check($v, $schema->items, $k_path, $i);
}

if (($type === 'string' && !is_string($v))
|| ($type === 'number' && !(is_numeric($v) && !is_string($v)))
|| ($type === 'integer' && !is_int($v))
){
$this->addError($k_path, ucwords(gettype($v)) . " value found, but $type is required", 'type');
} else {
$validator->check($v, $schema, $k_path, $i);
}
$validator->check($v, $schema->items, $k_path, $i);
}
$this->addErrors($typeValidator->getErrors());
$this->addErrors($validator->getErrors());
} else {
foreach ($value as $k => $v) {
$initErrors = $this->getErrors();

// First check if its defined in "items"
$this->checkUndefined($v, $schema->items, $path, $k);
$this->checkUndefined($v, $schema->items, $path, $k, $coerce);

// Recheck with "additionalItems" if the first test fails
if (count($initErrors) < count($this->getErrors()) && (isset($schema->additionalItems) && $schema->additionalItems !== false)) {
Expand All @@ -114,27 +129,27 @@ protected function validateItems($value, $schema = null, JsonPointer $path = nul
// Defined item type definitions
foreach ($value as $k => $v) {
if (array_key_exists($k, $schema->items)) {
$this->checkUndefined($v, $schema->items[$k], $path, $k);
$this->checkUndefined($v, $schema->items[$k], $path, $k, $coerce);
} else {
// Additional items
if (property_exists($schema, 'additionalItems')) {
if ($schema->additionalItems !== false) {
$this->checkUndefined($v, $schema->additionalItems, $path, $k);
$this->checkUndefined($v, $schema->additionalItems, $path, $k, $coerce);
} else {
$this->addError(
$path, 'The item ' . $i . '[' . $k . '] is not defined and the definition does not allow additional items', 'additionalItems', array('additionalItems' => $schema->additionalItems,));
}
} else {
// Should be valid against an empty schema
$this->checkUndefined($v, new \stdClass(), $path, $k);
$this->checkUndefined($v, new \stdClass(), $path, $k, $coerce);
}
}
}

// Treat when we have more schema definitions than values, not for empty arrays
if (count($value) > 0) {
for ($k = count($value); $k < count($schema->items); $k++) {
$this->checkUndefined($this->factory->createInstanceFor('undefined'), $schema->items[$k], $path, $k);
$this->checkUndefined($this->factory->createInstanceFor('undefined'), $schema->items[$k], $path, $k, $coerce);
}
}
}
Expand Down
37 changes: 28 additions & 9 deletions src/JsonSchema/Constraints/Constraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ abstract class Constraint implements ConstraintInterface

const CHECK_MODE_NORMAL = 0x00000001;
const CHECK_MODE_TYPE_CAST = 0x00000002;
const CHECK_MODE_COERCE = 0x00000004;

/**
* @var Factory
Expand Down Expand Up @@ -124,11 +123,16 @@ protected function incrementPath(JsonPointer $path = null, $i)
* @param mixed $schema
* @param JsonPointer|null $path
* @param mixed $i
* @param boolean $coerce
*/
protected function checkArray($value, $schema = null, JsonPointer $path = null, $i = null)
protected function checkArray(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing PHPDoc for parameter$coerse.
IMHO, $coerse flag would look better as the Factory field rather than additional method parameter.

Copy link
Collaborator Author

@shmax shmax Dec 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, good eye, thank you. I've added phpdocs, following the patterns and style already in place.

As for adding $coerce as a Factory flag, well, that was the approach I tried previously. It made sense in my previous approach because I was driving everything through check, and it just became a mode flag (stored in Factory, as you suggest). The problem there was that I had no reference to work with in check, so I couldn't properly coerce in all situations.

Now that I'm driving it through an explicit function call, coerce, it's no longer really a configuration value. If one were to, say, call check immediately after calling coerce, I would just have to set any stored fields back to false, so it would be another moving part with no real value.

I was actually interested in your reaction to what I did in CollectionConstraint. Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Changes look reasonable. Type check is more solid with this approach.
The performance penalty is not too big, avg time for validating 100K string array:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review! If you're satisfied, please approve.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no write permissions, can't approve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A LGTM will help

{
$validator = $this->factory->createInstanceFor('collection');
$validator->check($value, $schema, $path, $i);
if($coerce) {
$validator->coerce($value, $schema, $path, $i);
} else {
$validator->check($value, $schema, $path, $i);
}

$this->addErrors($validator->getErrors());
}
Expand All @@ -141,11 +145,16 @@ protected function checkArray($value, $schema = null, JsonPointer $path = null,
* @param JsonPointer|null $path
* @param mixed $i
* @param mixed $patternProperties
* @param boolean $coerce
*/
protected function checkObject($value, $schema = null, JsonPointer $path = null, $i = null, $patternProperties = null)
protected function checkObject(&$value, $schema = null, JsonPointer $path = null, $i = null, $patternProperties = null, $coerce = false)
{
$validator = $this->factory->createInstanceFor('object');
$validator->check($value, $schema, $path, $i, $patternProperties);
if($coerce){
$validator->coerce($value, $schema, $path, $i, $patternProperties);
} else {
$validator->check($value, $schema, $path, $i, $patternProperties);
}

$this->addErrors($validator->getErrors());
}
Expand All @@ -157,11 +166,16 @@ protected function checkObject($value, $schema = null, JsonPointer $path = null,
* @param mixed $schema
* @param JsonPointer|null $path
* @param mixed $i
* @param boolean $coerce
*/
protected function checkType($value, $schema = null, JsonPointer $path = null, $i = null)
protected function checkType(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)
{
$validator = $this->factory->createInstanceFor('type');
$validator->check($value, $schema, $path, $i);
if($coerce) {
$validator->coerce($value, $schema, $path, $i);
} else {
$validator->check($value, $schema, $path, $i);
}

$this->addErrors($validator->getErrors());
}
Expand All @@ -173,12 +187,17 @@ protected function checkType($value, $schema = null, JsonPointer $path = null, $
* @param mixed $schema
* @param JsonPointer|null $path
* @param mixed $i
* @param boolean $coerce
*/
protected function checkUndefined($value, $schema = null, JsonPointer $path = null, $i = null)
protected function checkUndefined(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)
{
$validator = $this->factory->createInstanceFor('undefined');

$validator->check($value, $this->factory->getSchemaStorage()->resolveRefSchema($schema), $path, $i);
if($coerce){
$validator->coerce($value, $this->factory->getSchemaStorage()->resolveRefSchema($schema), $path, $i);
} else {
$validator->check($value, $this->factory->getSchemaStorage()->resolveRefSchema($schema), $path, $i);
}

$this->addErrors($validator->getErrors());
}
Expand Down
117 changes: 23 additions & 94 deletions src/JsonSchema/Constraints/ObjectConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,23 @@
*/
class ObjectConstraint extends Constraint
{
/**
* {@inheritDoc}
*/
public function check($element, $definition = null, JsonPointer $path = null, $additionalProp = null, $patternProperties = null)
/**
* {@inheritDoc}
*/
public function check($element, $definition = null, JsonPointer $path = null, $additionalProp = null, $patternProperties = null)
{
$this->_check($element, $definition, $path, $additionalProp, $patternProperties);
}

/**
* {@inheritDoc}
*/
public function coerce(&$element, $definition = null, JsonPointer $path = null, $additionalProp = null, $patternProperties = null)
{
$this->_check($element, $definition, $path, $additionalProp, $patternProperties, true);
}

protected function _check(&$element, $definition = null, JsonPointer $path = null, $additionalProp = null, $patternProperties = null, $coerce = false)
{
if ($element instanceof UndefinedConstraint) {
return;
Expand All @@ -35,7 +48,7 @@ public function check($element, $definition = null, JsonPointer $path = null, $a

if ($definition) {
// validate the definition properties
$this->validateDefinition($element, $definition, $path);
$this->validateDefinition($element, $definition, $path, $coerce);
}

// additional the element properties
Expand Down Expand Up @@ -119,105 +132,21 @@ public function validateElement($element, $matches, $objectDefinition = null, Js
* @param \stdClass $element Element to validate
* @param \stdClass $objectDefinition ObjectConstraint definition
* @param JsonPointer|null $path Path?
* @param boolean $coerce Whether to coerce strings to expected types or not
*/
public function validateDefinition($element, $objectDefinition = null, JsonPointer $path = null)
public function validateDefinition(&$element, $objectDefinition = null, JsonPointer $path = null, $coerce = false)
{
$undefinedConstraint = $this->factory->createInstanceFor('undefined');

foreach ($objectDefinition as $i => $value) {
$property = $this->getProperty($element, $i, $undefinedConstraint);
$property = &$this->getProperty($element, $i, $undefinedConstraint);
$definition = $this->getProperty($objectDefinition, $i);

if($this->factory->getCheckMode() & Constraint::CHECK_MODE_TYPE_CAST){
if(!($property instanceof Constraint)) {
$property = $this->coerce($property, $definition);

if($this->factory->getCheckMode() & Constraint::CHECK_MODE_COERCE) {
if (is_object($element)) {
$element->{$i} = $property;
} else {
$element[$i] = $property;
}
}
}
}

if (is_object($definition)) {
// Undefined constraint will check for is_object() and quit if is not - so why pass it?
$this->checkUndefined($property, $definition, $path, $i);
}
}
}

/**
* Converts a value to boolean. For example, "true" becomes true.
* @param $value The value to convert to boolean
* @return bool|mixed
*/
protected function toBoolean($value)
{
if($value === "true"){
return true;
}

if($value === "false"){
return false;
}

return $value;
}

/**
* Converts a numeric string to a number. For example, "4" becomes 4.
*
* @param mixed $value The value to convert to a number.
* @return int|float|mixed
*/
protected function toNumber($value)
{
if(is_numeric($value)) {
return $value + 0; // cast to number
}

return $value;
}

protected function toInteger($value)
{
if(is_numeric($value) && (int)$value == $value) {
return (int)$value; // cast to number
}

return $value;
}

/**
* Given a value and a definition, attempts to coerce the value into the
* type specified by the definition's 'type' property.
*
* @param mixed $value Value to coerce.
* @param \stdClass $definition A definition with information about the expected type.
* @return bool|int|string
*/
protected function coerce($value, $definition)
{
$types = isset($definition->type)?$definition->type:null;
if($types){
foreach((array)$types as $type) {
switch ($type) {
case "boolean":
$value = $this->toBoolean($value);
break;
case "integer":
$value = $this->toInteger($value);
break;
case "number":
$value = $this->toNumber($value);
break;
}
$this->checkUndefined($property, $definition, $path, $i, $coerce);
}
}
return $value;
}

/**
Expand All @@ -229,7 +158,7 @@ protected function coerce($value, $definition)
*
* @return mixed
*/
protected function getProperty($element, $property, $fallback = null)
protected function &getProperty(&$element, $property, $fallback = null)
{
if (is_array($element) && (isset($element[$property]) || array_key_exists($property, $element)) /*$this->checkMode == self::CHECK_MODE_TYPE_CAST*/) {
return $element[$property];
Expand Down
Loading