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.

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

Add support for type coercion #308

Merged
merged 9 commits into from Oct 9, 2016
Merged

Add support for type coercion #308

merged 9 commits into from Oct 9, 2016

Conversation

shmax
Copy link
Collaborator

@shmax shmax commented Sep 15, 2016

For people (like myself) that use JSON Schema to validate API requests, a common problem is handling values that come across HTTP as strings:

$query = http_build_query([
   'processRefund'=>true,
   'refundAmount'=>17
]);

echo $query; // processRefund=true&refundAmount=17;

parse_str($query);
echo $processRefund; // "true" (a string value)
echo $refundAmount; // "17" (also a string value).

This will of course fail a json schema definition expecting booleans and numbers.

This change adds a new CHECK_MODE_COERCE mode that goes one step further than CHECK_MODE_TYPE_CAST by forcing strings to their expected types when appropriate in the original object so it can then be used naturally by the consuming client code.

$request = (object)[
   'processRefund'=>"true",
   'refundAmount'=>"17"
];

$checkMode = \JsonSchema\Constraints\Constraint::CHECK_MODE_COERCE | JsonSchema\Constraints\Constraint::CHECK_MODE_TYPE_CAST;
$validator = new \JsonSchema\Validator($checkMode);
$validator->check($request, (object) [
    "type"=>"object",
    "properties"=>[
        "processRefund"=>[
            "type"=>"boolean"
        ],
        "refundAmount"=>[
            "type"=>"number"
        ]
    ]
]); // validates!

is_bool($request->processRefund); // true
is_int($request->refundAmount); // true

This is a feature you see in some other validation libraries (https://github.com/geraintluff/jsv4-php, https://github.com/epoberezkin/ajv ), but if you guys aren't interested it's not a problem--I really have to compliment the injection model you've developed. It would be just as easy for me to add this functionality to my own sub-classes.

@shmax
Copy link
Collaborator Author

shmax commented Sep 19, 2016

Thoughts? Comments? Suggestions?

Copy link
Collaborator

@bighappyface bighappyface left a comment

Choose a reason for hiding this comment

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

Tests please

@shmax
Copy link
Collaborator Author

shmax commented Sep 20, 2016

@bighappyface Added tests. Thank you, that was a helpful exercise, and it prompted some further refinement:

  • added casting for the integer type
  • I've modified the mode member to be a bit mask, int/number/boolean casting is now triggered by the CHECK_MODE_TYPE_CAST flag, and CHECK_MODE_COERCE just allows the write to the input (because someone might want to allow the looser type checking, but not actually coerce the result).

@shmax
Copy link
Collaborator Author

shmax commented Sep 24, 2016

Please review, folks.

@@ -28,8 +28,9 @@
protected $errors = array();
protected $inlineSchemaProperty = '$schema';

const CHECK_MODE_NORMAL = 1;
const CHECK_MODE_TYPE_CAST = 2;
const CHECK_MODE_NORMAL = 0x00000001;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why hex literals?

Copy link
Collaborator Author

@shmax shmax Sep 24, 2016

Choose a reason for hiding this comment

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

Just a style thing I learned decades ago. I guess the idea is that this way it's a little more clear to the reader that we're creating a sequence for the purpose of bit-masking (and you don't have to get out a calculator to figure out what the higher numbers are, though it's not an issue in this simple case).

0x00000001
0x00000002
0x00000004
0x00000008
0x00000010
0x00000020
0x00000040

vs

1
2
4
8
16
32
64

But I could go either way if you feel strongly about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with it but I would caution that web developers, in my experience, are not usually accustomed to bitwise operations or thinking.

would go with a boolean literal if that is the clarity you intend to convey:

0b0000001 // 1
0b0000010 // 2
0b0000100 // 4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Went with binary literals.

protected function toInteger($value)
{
if(ctype_digit ($value)) {
return $value + 0; // cast to number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not cast instead of coerce 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.

Can you explain what it is you have in mind in a bit more detail, please? This is a cast (in the case that a string is input). The actual coercing happens (optionally) in lines 136 and 138 of this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am referring to an explicit cast instead of an operation that results in an implicit cast:

return (int) $value;

For the toNumber method it makes sense to use the operation to produce an integer or a float, but for a whole number simply casting to the desired type is more expressive, in my opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see no problem with that. Changed.

case "boolean":
$value = $this->toBoolean($value);
break;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead space

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@@ -146,6 +232,7 @@ protected function getProperty($element, $property, $fallback = null)
if (is_array($element) /*$this->checkMode == self::CHECK_MODE_TYPE_CAST*/) {
return array_key_exists($property, $element) ? $element[$property] : $fallback;
} elseif (is_object($element)) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead space

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@shmax
Copy link
Collaborator Author

shmax commented Sep 26, 2016

@bighappyface I had to go back to the hex literals; the binary integer literals aren't supported until PHP 5.4 , so the tests were failing. Hope that's not a problem.

http://fr2.php.net/manual/en/language.types.integer.php

@shmax
Copy link
Collaborator Author

shmax commented Sep 29, 2016

So what happens now?

@shmax shmax mentioned this pull request Oct 3, 2016
@shmax
Copy link
Collaborator Author

shmax commented Oct 5, 2016

@bighappyface Hey there, I submitted this PR 3 weeks ago, now. I'm not really complaining, but I was just curious what the merge process is? Are we waiting for someone or something? I have another small change I'd like to submit, but if this is a dead project I don't want to waste the effort. Let me know! Thanks...

@bighappyface bighappyface merged commit a918d3b into justinrainbow:master Oct 9, 2016
@shmax
Copy link
Collaborator Author

shmax commented Oct 9, 2016

Thanks much! 👍

@shmax shmax deleted the add-coercion-support branch October 9, 2016 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants