Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
195193002:
Add regular expression support in json_schema component (Closed)
|
Created:
6 years, 9 months ago by binjin Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd regular expression support in json_schema component
Add support of "pattern" and "patternProperties" attribute into json_schema, with third_party/re2 as regular expression implementation.
BUG=348551
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259943
Patch Set 1 #
Total comments: 19
Patch Set 2 : minor fixes #Patch Set 3 : two major changes #
Total comments: 11
Patch Set 4 : fixes #Patch Set 5 : fix a mistake #
Total comments: 21
Patch Set 6 : fixes #
Total comments: 15
Patch Set 7 : fixes #
Total comments: 2
Patch Set 8 : fix comment #
Messages
Total messages: 26 (0 generated)
Hello kalman, Could you have a look at this CL? Thanks -bjin
Joao should have a look at this as well, I think he knows the code better than me. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:193: if (!re2::RE2(string_value).ok()) { How often does Validate get called? If it's a lot it might be valuable to cache the compiled expressions since compilation, I presume, is expensive. But the memory tradeoff (whatever that may be) may not be worth it. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:194: *error = "Invalid regular expression in pattern attribute"; Could you include the RE2 error message here? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/re2/re... https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:219: for (base::DictionaryValue::Iterator it(*dictionary_value); huh, how does the |it| declaration here not kill the outer |it| variable... I didn't know C++ let you do that. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:222: *error = "Invalid regular expression in patternProperties attribute"; ditto error message https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:226: *error = "Invalid value for patternProperties attribute"; How about "patternProperties must be a dictionary" or something? https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:621: if (pattern_properties) { usual style I see would be to have the schema->GetDictionary as part of the conditional here. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:625: CHECK(prop_pattern->ok()); See comment above about caching these. In fact could you basically cache all of this block in a sensible way? It seems wasteful (and a bit odd to read) to be re-parsing it all. Perhaps at least share code with the parsing stage, e.g. a function which populates a couple of vectors and returns an error. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:642: for (size_t index = 0; index < pattern_properties_pattern.size(); index++) { ++index
https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:219: for (base::DictionaryValue::Iterator it(*dictionary_value); On 2014/03/11 14:53:39, kalman wrote: > huh, how does the |it| declaration here not kill the outer |it| variable... I > didn't know C++ let you do that. IIUC this is the same as shadowing an outer variable, and some compilers issue a warning about this. I think it's better to rename the iterator to avoid any confusion. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:649: } The order of these checks is not correct. See http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.3: "If the pattern matches the name of a property on the instance object, the value of the instance's property MUST be valid against the pattern name's schema value." And for additionalProperties: "This attribute defines a schema for all properties that are not explicitly defined in an object type definition." So from my understanding the right validation is: - for each pattern that matches a property key, the property's value must match the schema from that pattern; - then if the key matches a known property, it must match its schema; - else if the key didn't match any patterns, then it must match the additionalProperties (if they exist) In pseudo code (this is just to make this more concrete, other implementations are possibly better): ValidateObject(instance, schema) { properties = schema->GetDictionary(kProperties); additionalProperties = schema->GetDictionary(additionalProperties); patternProperties = schema->GetDictionary(patternProperties); vector<RE2*> patterns; vector<DictionaryValue*> pattern_schemas; for (pattern, value in patternProperties) { patterns.push_back(re2::RE2(pattern)); pattern_schemas.push_back(value); } // Validation starts here. for (key, value in instance) { bool defined_by_pattern = false; // Validate against any matching patterns. for (i from 0 to patterns.size()) { if (re2::RE2::PartialMatch(patterns[i], key)) { defined_by_pattern = true; Validate(value, pattern_schemas[i]); } } property_schema = properties->GetDictionary(key); if (property) { Validate(value, property_schema); } else if (!defined_by_schema && !SchemaAllowsAnyAdditionalProperties(schema)) { if (!additionalProperties) { errors_.push_back(kUnexpectedProperty); } else { Validate(value, additionalProperties); } } } } Can you also add tests that cover combinations of these cases?
https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:193: if (!re2::RE2(string_value).ok()) { On 2014/03/11 14:53:39, kalman wrote: > How often does Validate get called? If it's a lot it might be valuable to cache > the compiled expressions since compilation, I presume, is expensive. But the > memory tradeoff (whatever that may be) may not be worth it. Yes, regular expression compiling is not very cheap in re2 as far as I know. IsValidSchema() is a recursive function, but for each occurrence of "pattern" and "patternProperties" regular expression compilation will only be called once. This is a stand alone library, and typical usage is calling IsValidSchema() and then build a JSONSchemaValidator with it. I think it's reasonable to call IsValidSchema() only once. Compiled regular expression can't be passed as result in a simple way. It requires changes to IsValidSchema(), JSONSchemaValidator() and Validate(). On the other hand, caching will introduce memory overhead and will potential hurt the thread safety. Another option is not to check the correctness of regular expression in IsValidSchema() and assume invalid regular expression won't accept any string Later in JsonSchemaValidator::Validate(). It's not a ugly approach but will works well in practical. What do you think? https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:194: *error = "Invalid regular expression in pattern attribute"; On 2014/03/11 14:53:39, kalman wrote: > Could you include the RE2 error message here? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/re2/re... Done. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:219: for (base::DictionaryValue::Iterator it(*dictionary_value); On 2014/03/11 14:53:39, kalman wrote: > huh, how does the |it| declaration here not kill the outer |it| variable... I > didn't know C++ let you do that. My bad, I just copy-and-modified code from above block for "properties". Done(for this and above). https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:222: *error = "Invalid regular expression in patternProperties attribute"; On 2014/03/11 14:53:39, kalman wrote: > ditto error message Done. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:226: *error = "Invalid value for patternProperties attribute"; On 2014/03/11 14:53:39, kalman wrote: > How about "patternProperties must be a dictionary" or something? Done(for this and above). https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:621: if (pattern_properties) { On 2014/03/11 14:53:39, kalman wrote: > usual style I see would be to have the schema->GetDictionary as part of the > conditional here. Done(for this and above). https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:642: for (size_t index = 0; index < pattern_properties_pattern.size(); index++) { On 2014/03/11 14:53:39, kalman wrote: > ++index Done.
https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:649: } On 2014/03/11 16:20:13, Joao da Silva wrote: > The order of these checks is not correct. See > http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.3: > > "If the pattern matches the name of a property on the instance object, the value > of the instance's property MUST be valid against the pattern name's schema > value." > > And for additionalProperties: > > "This attribute defines a schema for all properties that are not explicitly > defined in an object type definition." > > So from my understanding the right validation is: > > - for each pattern that matches a property key, the property's value must match > the schema from that pattern; > - then if the key matches a known property, it must match its schema; > - else if the key didn't match any patterns, then it must match the > additionalProperties (if they exist) > > In pseudo code (this is just to make this more concrete, other implementations > are possibly better): > > ValidateObject(instance, schema) { > properties = schema->GetDictionary(kProperties); > additionalProperties = schema->GetDictionary(additionalProperties); > patternProperties = schema->GetDictionary(patternProperties); > > vector<RE2*> patterns; > vector<DictionaryValue*> pattern_schemas; > for (pattern, value in patternProperties) { > patterns.push_back(re2::RE2(pattern)); > pattern_schemas.push_back(value); > } > > // Validation starts here. > for (key, value in instance) { > bool defined_by_pattern = false; > // Validate against any matching patterns. > for (i from 0 to patterns.size()) { > if (re2::RE2::PartialMatch(patterns[i], key)) { > defined_by_pattern = true; > Validate(value, pattern_schemas[i]); > } > } > > property_schema = properties->GetDictionary(key); > if (property) { > Validate(value, property_schema); > } else if (!defined_by_schema && > !SchemaAllowsAnyAdditionalProperties(schema)) { > if (!additionalProperties) { > errors_.push_back(kUnexpectedProperty); > } else { > Validate(value, additionalProperties); > } > } > } > } > > Can you also add tests that cover combinations of these cases? I just read the specification, and from what I understood, if a property in instance match against both "properties" and "patternProperties", it must conform to both schema. So modification is still needed but it's much easier I think.
jumping to lunch, didn't get a chance to look at anything but the first comment sorry. https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/1/components/json_schema/json_... components/json_schema/json_schema_validator.cc:193: if (!re2::RE2(string_value).ok()) { On 2014/03/11 16:27:36, bjin wrote: > On 2014/03/11 14:53:39, kalman wrote: > > How often does Validate get called? If it's a lot it might be valuable to > cache > > the compiled expressions since compilation, I presume, is expensive. But the > > memory tradeoff (whatever that may be) may not be worth it. > > Yes, regular expression compiling is not very cheap in re2 as far as I know. > > IsValidSchema() is a recursive function, but for each occurrence of "pattern" > and "patternProperties" regular expression compilation will only be called once. > This is a stand alone library, and typical usage is calling IsValidSchema() and > then build a JSONSchemaValidator with it. I think it's reasonable to call > IsValidSchema() only once. > > Compiled regular expression can't be passed as result in a simple way. It > requires changes to IsValidSchema(), JSONSchemaValidator() and Validate(). On > the other hand, caching will introduce memory overhead and will potential hurt > the thread safety. > > Another option is not to check the correctness of regular expression in > IsValidSchema() and assume invalid regular expression won't accept any string > Later in JsonSchemaValidator::Validate(). It's not a ugly approach but will > works well in practical. > > What do you think? Good point re uncachability of this stuff. Ok. Your last comment makes sense.
that's all I have for now.
Hello, I just uploaded another patchset, containing two changes. 1) Do not check correctness of regular expressions in IsValidSchema, assuming invalid regex won't accept any strings later. 2) properties must conform to both "properties" and "patternProperties" schema if both are applicable. PTAL, thanks -bjin
The logic looks correct to me, just a couple more comments. https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:198: *error = "property must be a dictionary"; properties https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:614: << it.key() << "/."; LOG prop_pattern.error() and push it as an error too https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:626: nit: remove newline https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:771: << "/."; This should push an error too. Otherwise a typo will just silently accept any string, which is worse than rejecting all strings. https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:771: << "/."; Include compiled_regex.error() in the LOG and the error pushed too https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... File components/json_schema/json_schema_validator_unittest_base.cc (right): https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator_unittest_base.cc:244: instance->SetInteger("extra", 42); Isn't this already done in line 238 and tested in line 243?
Hello Joao, PTAL at these fixes. -bjin https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:198: *error = "property must be a dictionary"; On 2014/03/13 15:10:52, Joao da Silva wrote: > properties Done. https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:614: << it.key() << "/."; On 2014/03/13 15:10:52, Joao da Silva wrote: > LOG prop_pattern.error() and push it as an error too Done. https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:626: On 2014/03/13 15:10:52, Joao da Silva wrote: > nit: remove newline Done. https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator.cc:771: << "/."; On 2014/03/13 15:10:52, Joao da Silva wrote: > This should push an error too. Otherwise a typo will just silently accept any > string, which is worse than rejecting all strings. Done. https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... File components/json_schema/json_schema_validator_unittest_base.cc (right): https://codereview.chromium.org/195193002/diff/40001/components/json_schema/j... components/json_schema/json_schema_validator_unittest_base.cc:244: instance->SetInteger("extra", 42); On 2014/03/13 15:10:52, Joao da Silva wrote: > Isn't this already done in line 238 and tested in line 243? Done.
Small fixes left, then it's good from my side. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:215: *error = "patternProperty must be a dictionary"; patternProperties https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:325: "String must match the pattern: *."; Change this to: "String doesn't match pattern /*/: *" (see below) https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:335: "Regular expression /*/ in Schema is invalid."; Change this to: "Regular expression /*/ is invalid: *" (see below) https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:618: Error(path, FormatErrorMessage(kInvalidRegex, it.key()))); Include prop_pattern->error() in this Error (see kInvalidType for an example using two arguments) https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:776: Error(path, FormatErrorMessage(kInvalidRegex, pattern))); Include compiled_reger.error() in this Error too (same as above) https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:779: Error(path, FormatErrorMessage(kStringPattern, pattern))); Include compiled_reger.error() in this Error too (same as above) https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator.h (right): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.h:115: // For performance reason, currently IsValidSchema() won't check the For performance reasons https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.h:116: // correctness of regular expression used in "pattern" and "patternProperties" of regular expressions https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.h:117: // and in Validate() invalid regular expression don't accepts any strings. don't accept https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator_unittest_base.cc (left): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator_unittest_base.cc:122: if (type_ == CPP) You can remove "type_" from the .h file (this is causing a build failure on mac)
@Joao PTAL at the changes @kalman Could you take a look as well? -bjin https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:215: *error = "patternProperty must be a dictionary"; On 2014/03/21 10:15:16, Joao da Silva wrote: > patternProperties Done. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:325: "String must match the pattern: *."; On 2014/03/21 10:15:16, Joao da Silva wrote: > Change this to: "String doesn't match pattern /*/: *" (see below) I don't think the change is necessary, telling the pattern looks enough to me. More important, the tests reply strictly on error message, so it's quite possible that changes to re2 will break the tests. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:335: "Regular expression /*/ in Schema is invalid."; On 2014/03/21 10:15:16, Joao da Silva wrote: > Change this to: "Regular expression /*/ is invalid: *" (see below) Done. As a note I don't plan to add tests for this, since re2 might break the tests in unexpected way and the invalid regular expression is actually not allowed in json-schema so I think validating against such schema is not defined either. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:618: Error(path, FormatErrorMessage(kInvalidRegex, it.key()))); On 2014/03/21 10:15:16, Joao da Silva wrote: > Include prop_pattern->error() in this Error (see kInvalidType for an example > using two arguments) Done. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:776: Error(path, FormatErrorMessage(kInvalidRegex, pattern))); On 2014/03/21 10:15:16, Joao da Silva wrote: > Include compiled_reger.error() in this Error too (same as above) Done. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:779: Error(path, FormatErrorMessage(kStringPattern, pattern))); On 2014/03/21 10:15:16, Joao da Silva wrote: > Include compiled_reger.error() in this Error too (same as above) Not changed, as mentioned above. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator.h (right): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.h:115: // For performance reason, currently IsValidSchema() won't check the On 2014/03/21 10:15:16, Joao da Silva wrote: > For performance reasons Done. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.h:116: // correctness of regular expression used in "pattern" and "patternProperties" On 2014/03/21 10:15:16, Joao da Silva wrote: > of regular expressions Done. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.h:117: // and in Validate() invalid regular expression don't accepts any strings. On 2014/03/21 10:15:16, Joao da Silva wrote: > don't accept Done. https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator_unittest_base.cc (left): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator_unittest_base.cc:122: if (type_ == CPP) On 2014/03/21 10:15:16, Joao da Silva wrote: > You can remove "type_" from the .h file (this is causing a build failure on > mac) I think keep type field might still be useful for future reason, when CPP and JS implementation diverged on some tests. I added a getter to silence the compiler warnings, which might be useful later if there are both CPP and JS implementations.
lgtm https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/80001/components/json_schema/j... components/json_schema/json_schema_validator.cc:325: "String must match the pattern: *."; sgtm https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... File components/json_schema/json_schema_validator_unittest_base.h (right): https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator_unittest_base.h:32: ValidatorType type() { return type_; } What I meant is that "type_" isn't used anymore, so it can be removed as well as the enum and the ctor arg. I leave it to Benjamin to determine if it should be removed or not.
https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:194: it.value().GetAsDictionary(&dictionary_value); if it.value() isn't a DictionaryValue then |dictionary_value| will be untouched and just be whatever the last value is (which may or may not be NULL), right? i.e. you need to if (!it.value().GetAsDictionary(&dictinoary_value)) // fail ... and I guess, assuming I'm not misreading this, it was already a bug. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:211: it.value().GetAsDictionary(&dictionary_value); ... though this would have the same bug. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:214: if (!iter.value().GetAsDictionary(&dictionary_value)) { add a comment why you're not validating iter.key() as a regex https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:601: // Allowing any additional items will ignore pattern properties as well. I don't understand this comment, you're not ignoring pattern properties with |allow_any_additional_properties|? https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:620: kInvalidRegex, it.key(), prop_pattern->error()))); hm, why not continue after this? what is the point in adding an invalid pattern to this list? https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:636: if (pattern_properties_pattern[index]->ok() && (in which case you wouldn't need this check) https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... File components/json_schema/json_schema_validator_unittest_base.h (right): https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator_unittest_base.h:32: ValidatorType type() { return type_; } On 2014/03/21 15:06:40, Joao da Silva wrote: > What I meant is that "type_" isn't used anymore, so it can be removed as well as > the enum and the ctor arg. > > I leave it to Benjamin to determine if it should be removed or not. Why wouldn't it be removed?
Hello kalman, Please have a look at the changes. Thanks -bjin https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:194: it.value().GetAsDictionary(&dictionary_value); On 2014/03/21 15:27:20, kalman wrote: > if it.value() isn't a DictionaryValue then |dictionary_value| will be untouched > and just be whatever the last value is (which may or may not be NULL), right? > > i.e. you need to > > if (!it.value().GetAsDictionary(&dictinoary_value)) > // fail > > ... and I guess, assuming I'm not misreading this, it was already a bug. I think the type of it.value() have already been checked against corresponding entry in kExpectedTypes. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:211: it.value().GetAsDictionary(&dictionary_value); On 2014/03/21 15:27:20, kalman wrote: > ... though this would have the same bug. as noted above https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:214: if (!iter.value().GetAsDictionary(&dictionary_value)) { On 2014/03/21 15:27:20, kalman wrote: > add a comment why you're not validating iter.key() as a regex Done. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:601: // Allowing any additional items will ignore pattern properties as well. On 2014/03/21 15:27:20, kalman wrote: > I don't understand this comment, you're not ignoring pattern properties with > |allow_any_additional_properties|? Removed. In previous changeset there is a "if(allow_any_additional_properties)" statement to stop processing any further properties, which includes pattern properties. I forget to remove this comment after removing the if statement. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:620: kInvalidRegex, it.key(), prop_pattern->error()))); On 2014/03/21 15:27:20, kalman wrote: > hm, why not continue after this? what is the point in adding an invalid pattern > to this list? Done. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:636: if (pattern_properties_pattern[index]->ok() && On 2014/03/21 15:27:20, kalman wrote: > (in which case you wouldn't need this check) Done.
lgtm, thanks for persevering. https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/100001/components/json_schema/... components/json_schema/json_schema_validator.cc:194: it.value().GetAsDictionary(&dictionary_value); On 2014/03/24 14:35:09, bjin wrote: > On 2014/03/21 15:27:20, kalman wrote: > > if it.value() isn't a DictionaryValue then |dictionary_value| will be > untouched > > and just be whatever the last value is (which may or may not be NULL), right? > > > > i.e. you need to > > > > if (!it.value().GetAsDictionary(&dictinoary_value)) > > // fail > > > > ... and I guess, assuming I'm not misreading this, it was already a bug. > > I think the type of it.value() have already been checked against corresponding > entry in kExpectedTypes. ah, right. https://codereview.chromium.org/195193002/diff/120001/components/json_schema/... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/120001/components/json_schema/... components/json_schema/json_schema_validator.cc:210: // won't be checked here for performance reason. .... "reasons. Instead, invalid regular expressions will be caught as validation errors in Validate".
@joi: Could you take a look at components/test/data/json_schema/pattern_properties_dot.json ? Thanks -bjin https://codereview.chromium.org/195193002/diff/120001/components/json_schema/... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/195193002/diff/120001/components/json_schema/... components/json_schema/json_schema_validator.cc:210: // won't be checked here for performance reason. On 2014/03/25 14:38:06, kalman wrote: > .... "reasons. Instead, invalid regular expressions will be caught as validation > errors in Validate". Done.
//components LGTM
The CQ bit was checked by binjin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/195193002/150001
The CQ bit was unchecked by binjin@chromium.org
@brettw, I'm not sure the reason, but the presubmit bot insists on OWNER review from you, could you have a look at the DEPS file?
lgtm
The CQ bit was checked by binjin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/195193002/150001
Message was sent while issue was closed.
Change committed as 259943 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
