Improve error message display for Schema::Validate()

Improve error message display for Schema::Validate()

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

[binjin, 2014-01-23 15:39:41]

Hello Joao,

PTAL at this CL, thanks

Bin

[Joao da Silva, 2014-01-23 20:20:34]

Unfortunately the CL upload failed, it's not possible to see the diffs.

Can you upload again? It's a known problem that sometimes "git cl upload" fails
on the server side; you have to upload again when that happens.

[binjin, 2014-01-23 20:30:26]

On 2014/01/23 20:20:34, Joao da Silva wrote:
> Unfortunately the CL upload failed, it's not possible to see the diffs.
> 
> Can you upload again? It's a known problem that sometimes "git cl upload"
fails
> on the server side; you have to upload again when that happens.

Hello Joao,

I reuploaded the patch.

Bin

[Joao da Silva, 2014-01-23 20:59:31]

Looks fine, just a couple of comments.

Most of the changes needed are in formatting. If you haven't yet then I
recommend reviewing the style guides:

http://www.chromium.org/developers/coding-style

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

Clang-format will also do the right thing when long lines have to be split.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:108: void SchemaFindError(std::string*
error_path,std::string *error,
A better name for this is ErrorFound

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:109: const std::string& msg) {
Format this. One argument per line:

void SchemaFindError(std::string* error_path,
                     std::string* error,
                     const std::string& msg) {

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:117: if (path->size() == 0)
path->empty

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:120: *path =
base::StringPrintf("items[%d].", index) + *path;
Maybe just "[%d]" without the "items" looks OK?

E.g. "URLBlacklist[4].filter" instead of "URLBlacklist.items[4].filter"

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:126: if (path->size() == 0)
path->empty

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:677: "The value type doesn't match the
schema type.");
Format this line.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:695: StrategyForNextLevel(strategy),
error_path, error)) {
Format this line

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:749: "The value type doesn't match the
schema type.");
Format this line

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.h:83: // detailed message if |value| don't
conform the schema strictly. If |value|
"if |value| doesn't strictly conform to the schema."

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.h:84: // don't conform the schema even
within the allowance of |strategy|, false
"doesn't conform to the schema [...]"

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.h:86: // corresponding error caused the
failure. |error_path| can be NULL and in
"[..] error that caused the failure."

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema_map.cc (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema_map.cc:72: << " because it's not valid: "
<< error;
It's a good idea to add the error path to this log

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:147: std::string error_path =
"NOT_WRITEN_YET";
Maybe "NOT_SET"?

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:751: 
Test that there are not errors here

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:780: 
Test no errors here

[binjin, 2014-01-23 21:21:58]

Hello Joao,

PTAL at the newest patch. I noticed that two comments on schema_unittest.cc is
missing, I'm not sure the reason but there are fixed as well.

Bin

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:108: void SchemaFindError(std::string*
error_path,std::string *error,
On 2014/01/23 20:59:31, Joao da Silva wrote:
> A better name for this is ErrorFound

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:109: const std::string& msg) {
On 2014/01/23 20:59:31, Joao da Silva wrote:
> Format this. One argument per line:
> 
> void SchemaFindError(std::string* error_path,
>                      std::string* error,
>                      const std::string& msg) {

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:117: if (path->size() == 0)
On 2014/01/23 20:59:31, Joao da Silva wrote:
> path->empty

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:120: *path =
base::StringPrintf("items[%d].", index) + *path;
On 2014/01/23 20:59:31, Joao da Silva wrote:
> Maybe just "[%d]" without the "items" looks OK?
> 
> E.g. "URLBlacklist[4].filter" instead of "URLBlacklist.items[4].filter"

root schema can be a list, and things like "[4].first" looks odd to me.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:126: if (path->size() == 0)
On 2014/01/23 20:59:31, Joao da Silva wrote:
> path->empty

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:677: "The value type doesn't match the
schema type.");
On 2014/01/23 20:59:31, Joao da Silva wrote:
> Format this line.

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:695: StrategyForNextLevel(strategy),
error_path, error)) {
On 2014/01/23 20:59:31, Joao da Silva wrote:
> Format this line

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.cc:749: "The value type doesn't match the
schema type.");
On 2014/01/23 20:59:31, Joao da Silva wrote:
> Format this line

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.h:83: // detailed message if |value| don't
conform the schema strictly. If |value|
On 2014/01/23 20:59:31, Joao da Silva wrote:
> "if |value| doesn't strictly conform to the schema."

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.h:84: // don't conform the schema even
within the allowance of |strategy|, false
On 2014/01/23 20:59:31, Joao da Silva wrote:
> "doesn't conform to the schema [...]"

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema.h:86: // corresponding error caused the
failure. |error_path| can be NULL and in
On 2014/01/23 20:59:31, Joao da Silva wrote:
> "[..] error that caused the failure."

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema_map.cc (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema_map.cc:72: << " because it's not valid: "
<< error;
On 2014/01/23 20:59:31, Joao da Silva wrote:
> It's a good idea to add the error path to this log

Done.

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/139853013/diff/250001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:147: std::string error_path =
"NOT_WRITEN_YET";
On 2014/01/23 20:59:31, Joao da Silva wrote:
> Maybe "NOT_SET"?

Done.

[Joao da Silva, 2014-01-24 09:58:38]

lgtm with a small detail left.

https://codereview.chromium.org/139853013/diff/390001/components/policy/core/...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/139853013/diff/390001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:751: 
What I meant is that we should test that there are no errors at this line.
Something like:

  TestSchemaValidationWithPath(subschema, root, "NOT_SET");

[binjin, 2014-01-24 10:03:24]

https://codereview.chromium.org/139853013/diff/390001/components/policy/core/...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/139853013/diff/390001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:751: 
On 2014/01/24 09:58:39, Joao da Silva wrote:
> What I meant is that we should test that there are no errors at this line.
> Something like:
> 
>   TestSchemaValidationWithPath(subschema, root, "NOT_SET");

I think TestSchemaValidation(subschema, root, SCHEMA_STRICT, true) already did
this work. Also I used ASSERT_FALSE(returned) in TestSchemaValidation(), and
TestSchemaValidation() actually expect failure for input.

[Joao da Silva, 2014-01-27 10:35:52]

This is ready to go from my side.

https://codereview.chromium.org/139853013/diff/390001/components/policy/core/...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/139853013/diff/390001/components/policy/core/...
components/policy/core/common/schema_unittest.cc:751: 
On 2014/01/24 10:03:25, bjin wrote:
> On 2014/01/24 09:58:39, Joao da Silva wrote:
> > What I meant is that we should test that there are no errors at this line.
> > Something like:
> > 
> >   TestSchemaValidationWithPath(subschema, root, "NOT_SET");
> 
> I think TestSchemaValidation(subschema, root, SCHEMA_STRICT, true) already did
> this work. Also I used ASSERT_FALSE(returned) in TestSchemaValidation(), and
> TestSchemaValidation() actually expect failure for input.

OK, sgtm.

[commit-bot: I haz the power, 2014-01-27 10:36:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binjin@chromium.org/139853013/390001

[commit-bot: I haz the power, 2014-01-27 13:22:08]

Change committed as 247224