Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
205923004:
Add regex support in policy schema (Closed)
|
Created:
6 years, 9 months ago by binjin Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@json-schema-regex Visibility:
Public. |
DescriptionAdd regular expression support in policy schema.
This CL add "pattern" restriction to string type in schema, and "patternProperties" add variable properties name in an object type. Both of these features are based on regular expression and third_party/re2 is used in this implementation.
BUG=348551
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262364
Patch Set 1 #
Total comments: 36
Patch Set 2 : fixes #Patch Set 3 : Validate() against multiple properties #
Total comments: 39
Patch Set 4 : more fixes #
Total comments: 1
Patch Set 5 : #
Messages
Total messages: 16 (0 generated)
Hello Joao, PTAL at this CL, thanks. -bjin
First round of comments. I realize that this code is quite tricky, especially the generation part and the part that parses it. This probably needs more tests because they didn't catch 2 serious bugs: - schema.cc:1011: using begin->key instead of it->key - generate_policy_source.py:443: the order of the properties matters, and that code is inserting the properties of child schemas in the middle of the properties of the parent schema. Please add new tests that fail because of these bugs before fixing them. https://codereview.chromium.org/205923004/diff/1/components/policy/DEPS File components/policy/DEPS (right): https://codereview.chromium.org/205923004/diff/1/components/policy/DEPS#newcode2 components/policy/DEPS:2: "+third_party/re2", Add this in components/policy/core/DEPS https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.cc (left): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:476: // elements. Please leave this comment included. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:183: re2::RE2* CompileRegex(const std::string &pattern) const; const std::string& pattern https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:249: mutable std::map<std::string, re2::RE2*> regex_cache_; Argh, mutable. I think it's better than complicating this; please document why we use it though. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:422: sizes->string_enums ++; nit: remove space before ++ https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:555: *error = "Invalid regex: /" + it.key() + "/."; Include the ->error() https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:673: *error = "Invalid regex: /" + pattern + "/."; Include the ->error() in this message too https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:792: Schema subschema = GetProperty(it.key()); This needs to be updated to validate against all pattern schemas that apply. Something like: vector<Schema> pattern_schemas = GetPatternSchemas(it.key()); for (subschema in pattern_schemas) { if (!subschema.Validate()) // error } Schema subschema = GetProperty(it.key()); if (!subschema.valid() && pattern_schemas.empty()) { // Unknown property was detected. ... https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:868: Schema subschema = GetProperty(it.key()); Similar fixes here to handle GetPatternProperties() https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:1011: if (re2::RE2::PartialMatch(key, *storage_->CompileRegex(begin->key))) begin->key should be it->key https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:1017: Schema Schema::GetProperty(const std::string& key) const { So this method is just a helper to mix GetKnownProperty() and GetAdditionalProperties(), and it doesn't really work with GetPatternProperties() once that changes to return a list of matching schemas. I think this method should stay as is, but Validate() and Normalize() need to handle GetPatternProperties() correctly. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:1059: i < rnode->enumeration_restriction.offset_end; i++) { nit: add one space at the beginning of this line https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.h:153: Schema GetPatternProperty(const std::string& key) const; This should return a vector of Schemas, with all the matching schemas. Rename to GetPatternProperties() too and update the documentation. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema_internal.h (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:67: // right byond the last known pattern property. beyond https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:103: // For string type only, require |pattern_index| and |pattern_index_backup| requires https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:104: // to be exactly same. And it's an offset into SchemaData::string_enums to be exactly the same https://codereview.chromium.org/205923004/diff/1/components/policy/tools/gene... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/205923004/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:368: # here might be slightly different from re2. Leave a TODO to use a python wrapper for re2 here https://codereview.chromium.org/205923004/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:443: self.property_nodes.append((self.GetString(pattern), appending one by one and making recursive calls to Generate doesn't work (see the comment above). Basically you have a [begin, end) range, which must be contiguous. Now you're building an [end, pattern_end) range, which must also be contiguous; this means that all generated nodes must be appended together. However, the recursive call to Generate can append more nodes in between, and then the array will have the properties of different schemas mixed up. You have to first generate all of them, and then append them all together contiguously into property_nodes.
Hello Joao, Patchset 2 fixed most of the bugs(including the two you pointed out). For python generate bug, I tested it locally with the new ExtensionSettings policy I would like to introduce in following CLs. It's not included with this CL though. I'm still not sure about whether to follow json schema standard strictly or not. Following strictly will make Normalize() against multiple matched properties more complicated and make GetProperty() meaningless. -bjin https://codereview.chromium.org/205923004/diff/1/components/policy/DEPS File components/policy/DEPS (right): https://codereview.chromium.org/205923004/diff/1/components/policy/DEPS#newcode2 components/policy/DEPS:2: "+third_party/re2", On 2014/03/21 12:58:12, Joao da Silva wrote: > Add this in components/policy/core/DEPS Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.cc (left): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:476: // elements. On 2014/03/21 12:58:12, Joao da Silva wrote: > Please leave this comment included. Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:183: re2::RE2* CompileRegex(const std::string &pattern) const; On 2014/03/21 12:58:12, Joao da Silva wrote: > const std::string& pattern Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:249: mutable std::map<std::string, re2::RE2*> regex_cache_; On 2014/03/21 12:58:12, Joao da Silva wrote: > Argh, mutable. I think it's better than complicating this; please document why > we use it though. Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:422: sizes->string_enums ++; On 2014/03/21 12:58:12, Joao da Silva wrote: > nit: remove space before ++ Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:555: *error = "Invalid regex: /" + it.key() + "/."; On 2014/03/21 12:58:12, Joao da Silva wrote: > Include the ->error() Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:673: *error = "Invalid regex: /" + pattern + "/."; On 2014/03/21 12:58:12, Joao da Silva wrote: > Include the ->error() in this message too Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:1011: if (re2::RE2::PartialMatch(key, *storage_->CompileRegex(begin->key))) On 2014/03/21 12:58:12, Joao da Silva wrote: > begin->key should be it->key Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:1059: i < rnode->enumeration_restriction.offset_end; i++) { On 2014/03/21 12:58:12, Joao da Silva wrote: > nit: add one space at the beginning of this line Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema_internal.h (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:67: // right byond the last known pattern property. On 2014/03/21 12:58:12, Joao da Silva wrote: > beyond Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:103: // For string type only, require |pattern_index| and |pattern_index_backup| On 2014/03/21 12:58:12, Joao da Silva wrote: > requires Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:104: // to be exactly same. And it's an offset into SchemaData::string_enums On 2014/03/21 12:58:12, Joao da Silva wrote: > to be exactly the same Done. https://codereview.chromium.org/205923004/diff/1/components/policy/tools/gene... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/205923004/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:368: # here might be slightly different from re2. On 2014/03/21 12:58:12, Joao da Silva wrote: > Leave a TODO to use a python wrapper for re2 here Done. https://codereview.chromium.org/205923004/diff/1/components/policy/tools/gene... components/policy/tools/generate_policy_source.py:443: self.property_nodes.append((self.GetString(pattern), On 2014/03/21 12:58:12, Joao da Silva wrote: > appending one by one and making recursive calls to Generate doesn't work (see > the comment above). > > Basically you have a [begin, end) range, which must be contiguous. Now you're > building an [end, pattern_end) range, which must also be contiguous; this means > that all generated nodes must be appended together. > > However, the recursive call to Generate can append more nodes in between, and > then the array will have the properties of different schemas mixed up. > > You have to first generate all of them, and then append them all together > contiguously into property_nodes. Done.
Hello Joao, Could you please take a look at newest patchset? -bjin https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:792: Schema subschema = GetProperty(it.key()); On 2014/03/21 12:58:12, Joao da Silva wrote: > This needs to be updated to validate against all pattern schemas that apply. > Something like: > > vector<Schema> pattern_schemas = GetPatternSchemas(it.key()); > for (subschema in pattern_schemas) { > if (!subschema.Validate()) > // error > } > > Schema subschema = GetProperty(it.key()); > if (!subschema.valid() && pattern_schemas.empty()) { > // Unknown property was detected. > ... Done. But I think schema from pattern_properties and properties will both take effect if applicable. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:868: Schema subschema = GetProperty(it.key()); On 2014/03/21 12:58:12, Joao da Silva wrote: > Similar fixes here to handle GetPatternProperties() Done. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.cc:1017: Schema Schema::GetProperty(const std::string& key) const { On 2014/03/21 12:58:12, Joao da Silva wrote: > So this method is just a helper to mix GetKnownProperty() and > GetAdditionalProperties(), and it doesn't really work with > GetPatternProperties() once that changes to return a list of matching schemas. > > I think this method should stay as is, but Validate() and Normalize() need to > handle GetPatternProperties() correctly. I deprecated this function, as it's a little bit confusing since PatternProperties was introduced. https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/205923004/diff/1/components/policy/core/commo... components/policy/core/common/schema.h:153: Schema GetPatternProperty(const std::string& key) const; On 2014/03/21 12:58:12, Joao da Silva wrote: > This should return a vector of Schemas, with all the matching schemas. Rename to > GetPatternProperties() too and update the documentation. Done.
The logic looks all right from my side. This is ready to go after a couple more, smaller fixes. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:11: #include <vector> This is already in the .h https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:252: STLValueDeleter<std::map<std::string, re2::RE2*> > cache_deleter_; regex_cache_deleter_ https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:810: ++subschema) { These 2 lines can be joined https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:895: ++subschema) { These 2 lines can be joined https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:906: drop_it = true; Why not just push it here and break from the inner loop? https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:1085: i < rnode->enumeration_restriction.offset_end; i++) { ++i https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:1098: i < rnode->enumeration_restriction.offset_end; i++) { ++i https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:61: typedef std::vector<Schema> SchemaList; Put this outside the class. It will require a forward declaration: class Schema; typedef std::vector<Schema> SchemaList; class POLICY_EXPORT Schema { ... } https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:154: // Returns all Schemas from pattern properties that matchs |key|, might be [...] that match |key|. May be empty. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:166: Schema GetProperty(const std::string& key) const; Leave a TODO to remove this. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:169: // might be empty. Returns all Schemas that are supposed to be validated against for |key|. May be empty. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:16: #define TEST_SOURCE base::StringPrintf("%s:%i", __FILE__, __LINE__) An easier way to do this: #define TestSchemaValidation(a, b, c, d) \ TestSchemaValidationHelper(base::StringPrintf("%s: %i", __FILE__, __LINE__), a, b, c, d); If you do this then you don't need to pass TEST_SOURCE all over the place. Just an idea. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:506: { "bar+$", 4 }, // 9 nit: remove a space before the "4" to align the rest of the line https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:511: // patternProperties and 6 is the addionalProperties node. additionalProperties https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:588: ASSERT_EQ(static_cast<size_t>(1), schema_list.size()); 1u https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:888: base::StringValue(std::string("foobar")), You can pass a const char* directly to StringValue: base::StringValue("foobar") Same below. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:913: // Tests on ObjectWithPatternProperties Terminate sentence with . https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:919: ASSERT_EQ(static_cast<size_t>(1), 1u (also below) https://codereview.chromium.org/205923004/diff/40001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/tools/... components/policy/tools/generate_policy_source.py:290: # C/C++ escaped string. Can you document those differences? https://codereview.chromium.org/205923004/diff/40001/components/policy/tools/... components/policy/tools/generate_policy_source.py:369: # TODO(binjin): Add a python wrapper of re2 and use it here. Using ctypes should make this really easy: https://docs.python.org/2/library/ctypes.html The challenge is making sure that the GYP target that runs this script builds re2 first, and then passing the path to the built library to this script so that it can be loaded with ctypes.LoadLibrary. It may be tricky to get it working right on all platforms.
https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:11: #include <vector> On 2014/04/02 09:51:11, Joao da Silva wrote: > This is already in the .h Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:252: STLValueDeleter<std::map<std::string, re2::RE2*> > cache_deleter_; On 2014/04/02 09:51:11, Joao da Silva wrote: > regex_cache_deleter_ Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:810: ++subschema) { On 2014/04/02 09:51:11, Joao da Silva wrote: > These 2 lines can be joined Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:895: ++subschema) { On 2014/04/02 09:51:11, Joao da Silva wrote: > These 2 lines can be joined Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:906: drop_it = true; On 2014/04/02 09:51:11, Joao da Silva wrote: > Why not just push it here and break from the inner loop? Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:1085: i < rnode->enumeration_restriction.offset_end; i++) { On 2014/04/02 09:51:11, Joao da Silva wrote: > ++i Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.cc:1098: i < rnode->enumeration_restriction.offset_end; i++) { On 2014/04/02 09:51:11, Joao da Silva wrote: > ++i Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:61: typedef std::vector<Schema> SchemaList; On 2014/04/02 09:51:11, Joao da Silva wrote: > Put this outside the class. It will require a forward declaration: > > class Schema; > > typedef std::vector<Schema> SchemaList; > > class POLICY_EXPORT Schema { ... } Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:154: // Returns all Schemas from pattern properties that matchs |key|, might be On 2014/04/02 09:51:11, Joao da Silva wrote: > [...] that match |key|. May be empty. Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:166: Schema GetProperty(const std::string& key) const; On 2014/04/02 09:51:11, Joao da Silva wrote: > Leave a TODO to remove this. Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema.h:169: // might be empty. On 2014/04/02 09:51:11, Joao da Silva wrote: > Returns all Schemas that are supposed to be validated against for |key|. May be > empty. Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:16: #define TEST_SOURCE base::StringPrintf("%s:%i", __FILE__, __LINE__) On 2014/04/02 09:51:11, Joao da Silva wrote: > An easier way to do this: > > #define TestSchemaValidation(a, b, c, d) \ > TestSchemaValidationHelper(base::StringPrintf("%s: %i", __FILE__, __LINE__), > a, b, c, d); > > If you do this then you don't need to pass TEST_SOURCE all over the place. Just > an idea. Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:506: { "bar+$", 4 }, // 9 On 2014/04/02 09:51:11, Joao da Silva wrote: > nit: remove a space before the "4" to align the rest of the line Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:511: // patternProperties and 6 is the addionalProperties node. On 2014/04/02 09:51:11, Joao da Silva wrote: > additionalProperties Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:588: ASSERT_EQ(static_cast<size_t>(1), schema_list.size()); On 2014/04/02 09:51:11, Joao da Silva wrote: > 1u Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:888: base::StringValue(std::string("foobar")), On 2014/04/02 09:51:11, Joao da Silva wrote: > You can pass a const char* directly to StringValue: > > base::StringValue("foobar") > > Same below. Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:913: // Tests on ObjectWithPatternProperties On 2014/04/02 09:51:11, Joao da Silva wrote: > Terminate sentence with . Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:919: ASSERT_EQ(static_cast<size_t>(1), On 2014/04/02 09:51:11, Joao da Silva wrote: > 1u (also below) Done. https://codereview.chromium.org/205923004/diff/40001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/205923004/diff/40001/components/policy/tools/... components/policy/tools/generate_policy_source.py:290: # C/C++ escaped string. On 2014/04/02 09:51:11, Joao da Silva wrote: > Can you document those differences? Done.
Nice work here, thanks for handling all the comments. LGTM! https://codereview.chromium.org/205923004/diff/60001/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/205923004/diff/60001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:18: base::StringPrintf("%s:%i", __FILE__, __LINE__), a, b, c, d) nit: indent these 2 lines with 2 more spaces
@brettw: Could you please have a look at the changes to components/policy/core/DEPS ? Thanks. -bjin
@darin @cpu: Could any of you also take a quick look at components/policy/core/DEPS ? I'm adding a dependency on third_party/re2.
I don't see a problem with the dependency itself, lots of code depends on RE. Now the example itself in the bug is a bit alarming: validation of extensions ids is something that can be written very compactly. Are there more use cases? once you add this power it is going to be used for all sort of things and the troubleshooting burden will be high. Let me put it in another way. From my experience, once you introduce a secondary expressive language on a config system and you don't have a way to dry-run / debug / single-step such language you basically given the user a shotgun and it will be our support folks cleaning up the limbs.
On 2014/04/07 18:10:29, cpu wrote: > I don't see a problem with the dependency itself, lots of code depends on RE. > > Now the example itself in the bug is a bit alarming: validation of extensions > ids is something that can be written very compactly. > > Are there more use cases? once you add this power it is going to be used for all > sort of things and the troubleshooting burden will be high. Thanks for your review. I admit that there are not much other use cases, at least for "patternProperties". For "pattern" restriction on string types, I think there are some existing policies that can be applied to, like extension ids, domain name or email address. > > Let me put it in another way. From my experience, once you introduce a secondary > expressive language on a config system and you don't have a way to dry-run / > debug / single-step such language you basically given the user a shotgun and it > will be our support folks cleaning up the limbs. The regular expressions introduced by this CL is not visible to end users, all new regular expression will be added by chromium developers. Illegal regular expressions will fail complication since the python script will reject it. Wrong or mistyped regular expression will be caught in code review phase. For administrators, if they input a string that don't match the defined regex, they will get an error message just like the usual "Wrong type for policy value" one.
DEPS lgtm
ok. 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/205923004/80001
Message was sent while issue was closed.
Change committed as 262364 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
