Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
228423002:
Add $ref support to policy schema (Closed)
|
Created:
6 years, 8 months ago by binjin Modified:
6 years, 8 months ago Reviewers:
Joao da Silva CC:
chromium-reviews, joaodasilva+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@policy-schema-regex Visibility:
Public. |
DescriptionAdd $ref support to policy schema
BUG=347082
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262738
Patch Set 1 #Patch Set 2 : Use GetKnownProperty() in new tests #
Total comments: 18
Patch Set 3 : fixes #Patch Set 4 : Check type of $ref #
Total comments: 10
Patch Set 5 : fixes #
Messages
Total messages: 12 (0 generated)
Hello Joao, PTAL at this CL. Thanks -bjin
Hey Bin, I'm suggesting some big changes in this review. If you'd like some clarification please ask! Some things may not work directly the way I'm suggesting either, so please let me know if something won't work :-) https://codereview.chromium.org/228423002/diff/20001/components/policy/core/c... File components/policy/core/common/generate_policy_source_unittest.cc (right): https://codereview.chromium.org/228423002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:100: EXPECT_TRUE(subschema.GetKnownProperty("Idle").valid()); Let's have a more strict test for this: check that GetKnownProperty("Battery") equals GetKnownProperty("Delays"). You'll need to add a helper function in this test that compares if two Schema objects are the same. 2 Schema objects are equal if: - they are both invalid, OR: - they have the same type(), AND: - if the type() is LIST, then the schemas from GetItems() are equal too - if the type() is DICTIONARY, then: - have 2 iterators and iterate both at the same time; at each step they must have equal keys and schemas - GetAdditionalProperties() is also equal (maybe both are invalid) https://codereview.chromium.org/228423002/diff/20001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/228423002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:5289: 'id': 'ref_power_management_deleys', Change the id to "PowerManagementDelays" https://codereview.chromium.org/228423002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:5423: 'id': 'ref_device_login_screen_power_settings', Change the id to "DeviceLoginScreenPowerSettings" https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:410: return self.CheckID(schema, self.GetSimpleType(schema['type'])) I think adding CheckID() in the right places is hard to understand and it's easy to make a change in the future that breaks this. What do you think of adding a new function GenerateAndCollectID() that just wraps Generate like this: def GenerateAndCollectID(self, schema, name): index = self.Generate(schema, name) if schema.has_key('id'): assert type(index) == int # Check that schema doesn't have a $ref, check that ID doesn't exist yet, etc self.id_map[schema['id']] = index return index And then just replace all calls to Generate() with calls to GenerateAndCollectID()? https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:539: def ResolveReference(self): ResolveReferences https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:540: """ Resolve reference mapping, required to be called after Generate() Remove the space after """ https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:545: simple as looking up for corresponding index in self.id_map, and replace the corresponding ID https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:546: old index with it. old index with the mapped index. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:560: self.GetByID(additional), name) This is a bit more complicated than it needs to be; it'd be simpler if you change "schema_nodes", "property_nodes" and "properties_nodes" to have items as lists instead of tuples; then the elements can be mutated. You can also do the transformations quickly using map: from functools import partial def ResolveID(self, index, params): if isinstance(params[index], types.StringTypes): # Also check if the ID exists first... params[index] = self.id_map[params[index]] map(partial(self.ResolveID, 1), self.schema_nodes) map(partial(self.ResolveID, 1), self.property_nodes) map(partial(self.ResolveID, 3), self.properties_nodes)
https://codereview.chromium.org/228423002/diff/20001/components/policy/core/c... File components/policy/core/common/generate_policy_source_unittest.cc (right): https://codereview.chromium.org/228423002/diff/20001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:100: EXPECT_TRUE(subschema.GetKnownProperty("Idle").valid()); On 2014/04/08 15:02:10, Joao da Silva wrote: > Let's have a more strict test for this: check that GetKnownProperty("Battery") > equals GetKnownProperty("Delays"). You'll need to add a helper function in this > test that compares if two Schema objects are the same. > > 2 Schema objects are equal if: > - they are both invalid, OR: > - they have the same type(), AND: > - if the type() is LIST, then the schemas from GetItems() are equal too > - if the type() is DICTIONARY, then: > - have 2 iterators and iterate both at the same time; at each step they > must have equal keys and schemas > - GetAdditionalProperties() is also equal (maybe both are invalid) Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/228423002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:5289: 'id': 'ref_power_management_deleys', On 2014/04/08 15:02:10, Joao da Silva wrote: > Change the id to "PowerManagementDelays" Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:5423: 'id': 'ref_device_login_screen_power_settings', On 2014/04/08 15:02:10, Joao da Silva wrote: > Change the id to "DeviceLoginScreenPowerSettings" Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:410: return self.CheckID(schema, self.GetSimpleType(schema['type'])) On 2014/04/08 15:02:10, Joao da Silva wrote: > I think adding CheckID() in the right places is hard to understand and it's easy > to make a change in the future that breaks this. > > What do you think of adding a new function GenerateAndCollectID() that just > wraps Generate like this: > > def GenerateAndCollectID(self, schema, name): > index = self.Generate(schema, name) > if schema.has_key('id'): > assert type(index) == int > # Check that schema doesn't have a $ref, check that ID doesn't exist yet, > etc > self.id_map[schema['id']] = index > return index > > And then just replace all calls to Generate() with calls to > GenerateAndCollectID()? Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:539: def ResolveReference(self): On 2014/04/08 15:02:10, Joao da Silva wrote: > ResolveReferences Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:540: """ Resolve reference mapping, required to be called after Generate() On 2014/04/08 15:02:10, Joao da Silva wrote: > Remove the space after """ Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:545: simple as looking up for corresponding index in self.id_map, and replace the On 2014/04/08 15:02:10, Joao da Silva wrote: > corresponding ID Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:546: old index with it. On 2014/04/08 15:02:10, Joao da Silva wrote: > old index with the mapped index. Done. https://codereview.chromium.org/228423002/diff/20001/components/policy/tools/... components/policy/tools/generate_policy_source.py:560: self.GetByID(additional), name) On 2014/04/08 15:02:10, Joao da Silva wrote: > This is a bit more complicated than it needs to be; it'd be simpler if you > change "schema_nodes", "property_nodes" and "properties_nodes" to have items as > lists instead of tuples; then the elements can be mutated. > > You can also do the transformations quickly using map: > > from functools import partial > > def ResolveID(self, index, params): > if isinstance(params[index], types.StringTypes): > # Also check if the ID exists first... > params[index] = self.id_map[params[index]] > > map(partial(self.ResolveID, 1), self.schema_nodes) > map(partial(self.ResolveID, 1), self.property_nodes) > map(partial(self.ResolveID, 3), self.properties_nodes) Done.
lgtm after considering the suggestions inline https://codereview.chromium.org/228423002/diff/60001/components/policy/core/c... File components/policy/core/common/generate_policy_source_unittest.cc (right): https://codereview.chromium.org/228423002/diff/60001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:24: // consider restriction on integer or string, as well as pattern properties. Suggested improvement: "Checks if two schemas are the same or not. Note that this function doesn't consider restrictions on integers and strings nor pattern properties." https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:446: self.GenerateAndCollectID(subschema, key)) I think it's more readable if the tuple is all in the same line: properties = [ (self.GetString(key), self.GenerateAndCollectID(subschema, key)) for key, subschema in sorted_properties ] https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:476: attribute to self.id_map. This wrapper need to be used for every call to This wrapper needs https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:552: return tuple(lst) Suggestion: def ResolveID(self, index, params): return params[:index] + (self.GetByID(params[index]),) + params[index+1:] https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:558: might be either int or string. An int type suggest that it's a resolved suggests
https://codereview.chromium.org/228423002/diff/60001/components/policy/core/c... File components/policy/core/common/generate_policy_source_unittest.cc (right): https://codereview.chromium.org/228423002/diff/60001/components/policy/core/c... components/policy/core/common/generate_policy_source_unittest.cc:24: // consider restriction on integer or string, as well as pattern properties. On 2014/04/09 12:34:55, Joao da Silva wrote: > Suggested improvement: > > "Checks if two schemas are the same or not. Note that this function doesn't > consider restrictions on integers and strings nor pattern properties." Done. https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:446: self.GenerateAndCollectID(subschema, key)) On 2014/04/09 12:34:55, Joao da Silva wrote: > I think it's more readable if the tuple is all in the same line: > > properties = [ > (self.GetString(key), self.GenerateAndCollectID(subschema, key)) > for key, subschema in sorted_properties ] Done. https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:476: attribute to self.id_map. This wrapper need to be used for every call to On 2014/04/09 12:34:55, Joao da Silva wrote: > This wrapper needs Done. https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:552: return tuple(lst) On 2014/04/09 12:34:55, Joao da Silva wrote: > Suggestion: > > def ResolveID(self, index, params): > return params[:index] + (self.GetByID(params[index]),) + params[index+1:] Done. https://codereview.chromium.org/228423002/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:558: might be either int or string. An int type suggest that it's a resolved On 2014/04/09 12:34:55, Joao da Silva wrote: > suggests Done.
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/228423002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg
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/228423002/80001
Message was sent while issue was closed.
Change committed as 262738 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
