Implements a C++ version of JSONSchemaValidator.

Implements a C++ version of JSONSchemaValidator.

BUG=none
TEST=covered by unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65867

[Aaron Boodman, 2010-11-08 10:42:55]

I'm still finishing up the tests for this, but I think it's worth starting to
look at now.

[Aaron Boodman, 2010-11-08 10:44:34]

Forgot to say, the reason for the odd test setup is because there is also a
JavaScript-based version of this class. I want to ensure their implementations
are the same, so I plan to inherit a JSONSchemaValidatorTestJS from
JSONSchemaValidatorTestBase. But that will be a separate change.

[asargent_no_longer_on_chrome, 2010-11-08 23:47:18]

I haven't finished looking at the test code yet, but wanted to send out my
comments on JsonSchemaValidator. 

Apart from the specific comments, one thing you might think about is the use of
CHECK in various places for well-formedness of the schema. If you stick with
this strategy, at a minimum there should be a stern warning somewhere that the
schema is expected to be well-formed, and it's a fatal error if it isn't, and it
would be nice if we also provided a method a caller could use to check the
validity of a schema. Alternatively, you could take all the places that use
CHECK on various properties of the schema and make those just be new kinds of
validation errors.

http://codereview.chromium.org/4673001/diff/2001/3003
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/4673001/diff/2001/3003#newcode54
chrome/common/json_schema_validator.cc:54: return double_value >
floor(double_value) ? "number" : "integer";
Should integral doubles that are > INT_MAX have type "number" instead of
"integer"?

http://codereview.chromium.org/4673001/diff/2001/3003#newcode170
chrome/common/json_schema_validator.cc:170: types_[id] = schema;
What if the schema had 2 different type definitions with the same id? Shouldn't
that be an error? If so then check that types_ doesn't already have an entry for
id here.

http://codereview.chromium.org/4673001/diff/2001/3003#newcode232
chrome/common/json_schema_validator.cc:232: errors_.resize(original_num_errors);
It took me a moment to grok what you're doing with the resize of errors_ here -
to basically try validating against the schema of each choice in choices and
return if you get a no-error result. Consider adding a brief comment here,
and/or moving the resize of errors to the bottom of the body of the for loop
(that would also save the extra resize statement just outside the loop).

http://codereview.chromium.org/4673001/diff/2001/3003#newcode248
chrome/common/json_schema_validator.cc:248: choices->Get(i, &choice);
should you DCHECK that choice is not null after this call to Get?

http://codereview.chromium.org/4673001/diff/2001/3003#newcode357
chrome/common/json_schema_validator.cc:357: ListValue* tuple_type = NULL;
Consider splitting the rest of the code from here down into a separate method.

http://codereview.chromium.org/4673001/diff/2001/3004
File chrome/common/json_schema_validator.h (right):

http://codereview.chromium.org/4673001/diff/2001/3004#newcode20
chrome/common/json_schema_validator.h:20: class JSONSchemaValidator {
It would probably be worth having a top-level comment here about the class'
general purpose, and a few brief examples of json schema and valid/invalid
input. That might make it easier to understand for a casual reader not already
familiar with our extension_api.json.

http://codereview.chromium.org/4673001/diff/2001/3004#newcode55
chrome/common/json_schema_validator.h:55: static std::string
FormatErrorMessage(const std::string& format,
nit: Document what s1 (and below, s1,s2) are used for, or give them more
descriptive names. I'm guessing they're substitutions for tokens in the error
message.

http://codereview.chromium.org/4673001/diff/2001/3004#newcode65
chrome/common/json_schema_validator.h:65: // type must have an "id" property
that can be referred to within the schema
nit: In this comment, consider replacing
"Each type must have ..."
with
"Each type must be a DictionaryValue with .."

http://codereview.chromium.org/4673001/diff/2001/3004#newcode99
chrome/common/json_schema_validator.h:99: const std::string& path);
The way that paths work is a little bit subtle, and it might be worth adding
overview documentation somewhere.

http://codereview.chromium.org/4673001/diff/2001/3004#newcode104
chrome/common/json_schema_validator.h:104: void ValidateEnum(Value* instance,
ListValue* choices,
consider adding a comment here about the difference between ValidateChoices and
ValidateEnum, since they have the same method signature.

[Erik does not do reviews, 2010-11-09 18:34:21]

I didn't look at style too deeply or the details of all of the tests since
Antony's already on the review.  Overall, LGTM.  I'm very excited to see this
coming in.

http://codereview.chromium.org/4673001/diff/13001/11004
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/4673001/diff/13001/11004#newcode175
chrome/common/json_schema_validator.cc:175: if (schema->GetString("$ref", &ref))
{
one thing that's a little weird here is that it looks like It's possible to
define a schema with $ref, "choices", "enum", etc. all set.  Seems like these
should trigger an error of being a bogus schema.

http://codereview.chromium.org/4673001/diff/13001/11004#newcode189
chrome/common/json_schema_validator.cc:189: if (schema->GetList("choices",
&choices)) {
feels like these keys should be pulled out as constants

http://codereview.chromium.org/4673001/diff/13001/11004#newcode218
chrome/common/json_schema_validator.cc:218: else if (type != "boolean" && type
!= "null")
why don't we need a ValidateBoolean?

http://codereview.chromium.org/4673001/diff/13001/11004#newcode448
chrome/common/json_schema_validator.cc:448: if (GetNumberFromDictionary(schema,
"maximum", &maximum)) {
maybe add a CHECK that maximum > minimum

[Aaron Boodman, 2010-11-10 19:03:35]

Regarding the presence of fatal checks of schema-well-formedness: I think I
would like to handle that by eventually having a ValidateSchema() method that
uses a JSON schema for JSON schema (whee!).

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.cc:54: return double_value >
floor(double_value) ? "number" : "integer";
On 2010/11/08 23:47:18, Antony Sargent wrote:
> Should integral doubles that are > INT_MAX have type "number" instead of
> "integer"?

I don't think so. "integer" here just means "integral". I changed it to instead
check the max precisely representable integer.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.cc:170: types_[id] = schema;
On 2010/11/08 23:47:18, Antony Sargent wrote:
> What if the schema had 2 different type definitions with the same id?
Shouldn't
> that be an error? If so then check that types_ doesn't already have an entry
for
> id here. 

Good idea. Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.cc:232: errors_.resize(original_num_errors);
On 2010/11/08 23:47:18, Antony Sargent wrote:
> It took me a moment to grok what you're doing with the resize of errors_ here
-
> to basically try validating against the schema of each choice in choices and
> return if you get a no-error result. Consider adding a brief comment here,
> and/or moving the resize of errors to the bottom of the body of the for loop
> (that would also save the extra resize statement just outside the loop). 

Good ideas. Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.cc:248: choices->Get(i, &choice);
On 2010/11/08 23:47:18, Antony Sargent wrote:
> should you DCHECK that choice is not null after this call to Get?

Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.cc:357: ListValue* tuple_type = NULL;
On 2010/11/08 23:47:18, Antony Sargent wrote:
> Consider splitting the rest of the code from here down into a separate method.


good idea. done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
File chrome/common/json_schema_validator.h (right):

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.h:20: class JSONSchemaValidator {
On 2010/11/08 23:47:18, Antony Sargent wrote:
> It would probably be worth having a top-level comment here about the class'
> general purpose, and a few brief examples of json schema and valid/invalid
> input. That might make it easier to understand for a casual reader not already
> familiar with our extension_api.json.

Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.h:55: static std::string
FormatErrorMessage(const std::string& format,
On 2010/11/08 23:47:18, Antony Sargent wrote:
> nit: Document what s1 (and below, s1,s2) are used for, or give them more
> descriptive names. I'm guessing they're substitutions for tokens in the error
> message.

Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.h:65: // type must have an "id" property
that can be referred to within the schema
On 2010/11/08 23:47:18, Antony Sargent wrote:
> nit: In this comment, consider replacing
> "Each type must have ..."
> with
> "Each type must be a DictionaryValue with .." 

Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.h:99: const std::string& path);
On 2010/11/08 23:47:18, Antony Sargent wrote:
> The way that paths work is a little bit subtle, and it might be worth adding
> overview documentation somewhere. 

Done.

http://codereview.chromium.org/4673001/diff/2001/chrome/common/json_schema_va...
chrome/common/json_schema_validator.h:104: void ValidateEnum(Value* instance,
ListValue* choices,
On 2010/11/08 23:47:18, Antony Sargent wrote:
> consider adding a comment here about the difference between ValidateChoices
and
> ValidateEnum, since they have the same method signature.

Done.

http://codereview.chromium.org/4673001/diff/13001/chrome/common/json_schema_v...
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/4673001/diff/13001/chrome/common/json_schema_v...
chrome/common/json_schema_validator.cc:175: if (schema->GetString("$ref", &ref))
{
On 2010/11/09 18:34:21, Erik Kay wrote:
> one thing that's a little weird here is that it looks like It's possible to
> define a schema with $ref, "choices", "enum", etc. all set.  Seems like these
> should trigger an error of being a bogus schema.

You can define a schema like that, but we won't honor it. I think instead of
making the parser be super paranoid, we should define a schema for json schema
(whee!) and use that to validate.

http://codereview.chromium.org/4673001/diff/13001/chrome/common/json_schema_v...
chrome/common/json_schema_validator.cc:218: else if (type != "boolean" && type
!= "null")
On 2010/11/09 18:34:21, Erik Kay wrote:
> why don't we need a ValidateBoolean?

ValidateType() handles the basic 'do the types match' check, then we have
additional Validate*() methods for the types that have additional constraints
that can be set. Booleans don't have any additional constraints.

http://codereview.chromium.org/4673001/diff/13001/chrome/common/json_schema_v...
chrome/common/json_schema_validator.cc:448: if (GetNumberFromDictionary(schema,
"maximum", &maximum)) {
On 2010/11/09 18:34:21, Erik Kay wrote:
> maybe add a CHECK that maximum > minimum

Hm. That is a good idea, but I want to implement a Validate() method that checks
schemas using json schema itself, and json schema can't catch this kind of
error.

[Erik does not do reviews, 2010-11-10 19:11:06]

I'll defer to Antony on the rest.  LGTM

[asargent_no_longer_on_chrome, 2010-11-11 01:30:49]

lgtm w/ a couple of final nits

http://codereview.chromium.org/4673001/diff/18001/chrome/common/json_schema_v...
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/4673001/diff/18001/chrome/common/json_schema_v...
chrome/common/json_schema_validator.cc:177: types_[id] = schema;
I see you added the CHECK above in the constructor for non-duplicate type
definitions in the incoming types ListValue, but can't you still run into the
problem here for sub-definitions further down in the tree?

http://codereview.chromium.org/4673001/diff/18001/chrome/common/json_schema_v...
chrome/common/json_schema_validator.cc:244: // validations failed.
don't you mean "We only want to know if any of the validations succeeded" ?

http://codereview.chromium.org/4673001/diff/18001/chrome/common/json_schema_v...
File chrome/common/json_schema_validator_unittest_base.cc (right):

http://codereview.chromium.org/4673001/diff/18001/chrome/common/json_schema_v...
chrome/common/json_schema_validator_unittest_base.cc:59: }
nit: add "  // namespace" after the closing brace here

http://codereview.chromium.org/4673001/diff/18001/chrome/common/json_schema_v...
chrome/common/json_schema_validator_unittest_base.cc:229:
CHECK(schema->GetDictionary("properties", &properties));
If you are unlucky a certain chromium engineer who watches test code in CL's
closely will spot that you are using CHECK here and request you change it to
EXPECT or ASSERT. (Remaining nameless in case that might trigger a gmail filter)
Perhaps better to fend it off by changing in advance. :)