Added a validator for JSON v3 schemas.

Added a validator for JSON v3 schemas.

This function checks if the input string is a valid JSON v3 schema, with some
additional restrictions (e.g. "pattern" is not supported). The DictionaryValue
returned can be used to build a JSONSchemaValidator, if the check succeeds.

BUG=108992
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201473

[Joao da Silva, 2013-05-14 16:42:46]

So here we go :-) This is the first of ~5 CLs to implement the managed storage
schema.

@kalman: please review; is there someone else who should also have a look at
this from your side?

@mnissler: FYI

[not at google - send to devlin, 2013-05-16 17:08:20]

Sadly, I am probably the json schema person over here.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
File chrome/common/json_schema/json_schema_validator.cc (right):

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:74: continue;
the problem with this approach is that it won't inform of unsupported
properties, which is asking for typos. Could you invert this; have
kExpectedTypes be a map of schema value -> type and loop over |dict| instead?
That may also be more efficient.

Of course, making kExpectedTypes a map would require some kind of lazy
initialisation. You could do that, or do a trick and make kExpectedTypes sorted
and use binary_search on it, or something.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:86: base::StringPrintf("Value
of %s must be >= 0", kExpectedTypes[i].key);
might as well include integer_value in here.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:97: } else if
(value->GetAsString(&string_value)) {
else unnecessary after a return.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:102: } else if
(value->GetAsList(&list_value)) {
a switch might be nicer

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:122: return false;
all these times you validate IsValidSchema and then return without setting error
make sense - but it may be nice to DCHECK_NE("", *error) each time as well. Or
that may be overkill.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:158: case
base::Value::TYPE_DOUBLE:
nit: 1 case per line, this looks pretty odd.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:173: } else if
(!IsValidSchema(dictionary_value, error)) {
else unnecessary after a return

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:180: // Unsupported
attributes.
if you do the map thing right at the top then this wouldn't be necessary

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:303: return
make_scoped_ptr(dict);
does just

return json.PassAs<base::DictionaryValue>() work?

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
File chrome/common/json_schema/json_schema_validator.h (right):

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.h:109: std::string* error);
a static Create method which takes a std::string schema / error* and returns a
scoped_ptr<JSONSchemaValidator> would be easier to use. Transferring
dependencies (i.e. constructing a JSV depends on something that came from
IsValidSchema) is better factored that way.

However, it would probably require more invasive changes/restructuring to
json_schema_validator.cc.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
File chrome/common/json_schema/json_schema_validator_unittest.cc (right):

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator_unittest.cc:89: "{ \"type\":
[\"object\", \"array\"] }", &error)) << error;
test a list of items?

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator_unittest.cc:115: " 
\"unknown_key\": \"is ignored\""
hmm...

[Joao da Silva, 2013-05-19 13:16:29]

@kalman: PTAL

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
File chrome/common/json_schema/json_schema_validator.cc (right):

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:74: continue;
On 2013/05/16 17:08:20, kalman wrote:
> the problem with this approach is that it won't inform of unsupported
> properties, which is asking for typos. Could you invert this; have
> kExpectedTypes be a map of schema value -> type and loop over |dict| instead?
> That may also be more efficient.
> 

Good idea, done. "type" and "items" is still special-cased, since those
attributes have two legal types.

The flip-side of doing this is forward-compatibility, in case we want to support
other keys in the future. I've decided to do the stricter checks, since JSON
schema v3 is set in stone and the version is declared in the managed storage
schema; if we support a new format in the future then we can gate the checks on
the version.

Added two more attributes that were ignored before, but that we expect to see in
the schema ("title" and "description") to generate better UI and documentation
from the schema.

> Of course, making kExpectedTypes a map would require some kind of lazy
> initialisation. You could do that, or do a trick and make kExpectedTypes
sorted
> and use binary_search on it, or something.

An std::map is usually a red-black tree, so doing a binary search over a static
array yields the same logn lookup time. I find that simpler than adding a lazily
initialized std::map.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:86: base::StringPrintf("Value
of %s must be >= 0", kExpectedTypes[i].key);
On 2013/05/16 17:08:20, kalman wrote:
> might as well include integer_value in here.

Done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:97: } else if
(value->GetAsString(&string_value)) {
On 2013/05/16 17:08:20, kalman wrote:
> else unnecessary after a return.

Done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:102: } else if
(value->GetAsList(&list_value)) {
On 2013/05/16 17:08:20, kalman wrote:
> a switch might be nicer

Done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:122: return false;
On 2013/05/16 17:08:20, kalman wrote:
> all these times you validate IsValidSchema and then return without setting
error
> make sense - but it may be nice to DCHECK_NE("", *error) each time as well. Or
> that may be overkill.

That's a good idea, and there's a bug here too: if |it| is not a dictionary then
this fails without setting |error|. Fixed.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:158: case
base::Value::TYPE_DOUBLE:
On 2013/05/16 17:08:20, kalman wrote:
> nit: 1 case per line, this looks pretty odd.

Done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:173: } else if
(!IsValidSchema(dictionary_value, error)) {
On 2013/05/16 17:08:20, kalman wrote:
> else unnecessary after a return

Done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:180: // Unsupported
attributes.
On 2013/05/16 17:08:20, kalman wrote:
> if you do the map thing right at the top then this wouldn't be necessary

Yep, done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.cc:303: return
make_scoped_ptr(dict);
On 2013/05/16 17:08:20, kalman wrote:
> does just
> 
> return json.PassAs<base::DictionaryValue>() work?

It doesn't; PassAs<>() can only do upcasts.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
File chrome/common/json_schema/json_schema_validator.h (right):

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator.h:109: std::string* error);
On 2013/05/16 17:08:20, kalman wrote:
> a static Create method which takes a std::string schema / error* and returns a
> scoped_ptr<JSONSchemaValidator> would be easier to use. Transferring
> dependencies (i.e. constructing a JSV depends on something that came from
> IsValidSchema) is better factored that way.
> 

The returned DictionaryValue can be used to build a JSV, but for the
policy-for-extensions work it'll be used to build a PolicySchema instead. So I'd
leave the types signature as is, but I'm cool with moving this somewhere else or
renaming it, if you have better ideas.

> However, it would probably require more invasive changes/restructuring to
> json_schema_validator.cc.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
File chrome/common/json_schema/json_schema_validator_unittest.cc (right):

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator_unittest.cc:89: "{ \"type\":
[\"object\", \"array\"] }", &error)) << error;
On 2013/05/16 17:08:20, kalman wrote:
> test a list of items?

Done.

https://codereview.chromium.org/14830007/diff/1001/chrome/common/json_schema/...
chrome/common/json_schema/json_schema_validator_unittest.cc:115: " 
\"unknown_key\": \"is ignored\""
On 2013/05/16 17:08:20, kalman wrote:
> hmm...

Removed :-)

[not at google - send to devlin, 2013-05-20 21:08:52]

lgtm

https://codereview.chromium.org/14830007/diff/12001/chrome/common/json_schema...
File chrome/common/json_schema/json_schema_validator.cc (right):

https://codereview.chromium.org/14830007/diff/12001/chrome/common/json_schema...
chrome/common/json_schema/json_schema_validator.cc:126: static_cast<int>(i));
can you use %u and avoid the static_cast? or does stringprintf not support that?

[Joao da Silva, 2013-05-21 15:58:28]

https://codereview.chromium.org/14830007/diff/12001/chrome/common/json_schema...
File chrome/common/json_schema/json_schema_validator.cc (right):

https://codereview.chromium.org/14830007/diff/12001/chrome/common/json_schema...
chrome/common/json_schema/json_schema_validator.cc:126: static_cast<int>(i));
On 2013/05/20 21:08:52, kalman wrote:
> can you use %u and avoid the static_cast? or does stringprintf not support
that?

The problem is that size_t is 32 bits in some platforms and 64 in others, so
some expect %u and others %lu.

We should have some constant for this that could be concatenated into the
string, but we don't :-/

[commit-bot: I haz the power, 2013-05-21 15:58:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/14830007/12001

[commit-bot: I haz the power, 2013-05-21 23:22:25]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-22 06:42:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/14830007/12001

[commit-bot: I haz the power, 2013-05-22 09:00:48]

Change committed as 201473