Support API aliases

tools/json_schema_compiler/feature_compiler.py

Index: tools/json_schema_compiler/feature_compiler.py
diff --git a/tools/json_schema_compiler/feature_compiler.py b/tools/json_schema_compiler/feature_compiler.py
index 9e0b8fb6883c2a1116e1936c8568e4cc155780c3..6d31695cbd02c0cd661c686db1198bc6aa2c7b62 100644
--- a/tools/json_schema_compiler/feature_compiler.py
+++ b/tools/json_schema_compiler/feature_compiler.py
@@ -98,10 +98,28 @@ CC_FILE_END = """
 #                Feature::BLESSED_WEB_PAGE_CONTEXT et al for contexts. If not
 #                specified, defaults to false.
 #   'values': A list of all possible allowed values for a given key.
+#   'shared': Boolean that, if set, ensures that only one of the associated
+#       features has the feature property set. Used primarily for complex
+#       features - for simple features, there is always at most one feature
+#       setting an option.
+#   'feature_reference': Only applicable for strings. An object that, if

[Devlin, 2016/11/21 16:55:53]

This feature reference code looks like it adds a lot of complication.  I wonder
if a simpler (and potentially more flexible) way of doing it would be to have a
final validation that happens once we parse all our features and is called
similar to validation.  Then, we could do something like:


def IsFeatureCrossReference(first_key, second_key, feature, all_features):
  val = feature.GetValue(first_key)
  if not val:
    return True
  if not val in all_features:
    return False
  xref_feature = all_features[val]
  xref_val = xref_feature.GetValue(second_key)
  if not xref_val or xref_val is not feature.name:
    return False
  return True

FINAL_VALIDATION = ([
  'APIFeature': [
    (partial(IsFeatureCrossReference, 'alias', 'source'),
     '<error message>')
  ]
])

(continued below)

This would allow us to later add any other validations that require knowledge of
other features more easily.

[tbarzic, 2016/11/21 23:49:47]

Done (though the error message returned using this approach is slightly less
precise :/).

+#       provided, verifies that the value is set to a name of another feature
+#       defined in the same features file. It can following options:
+#           'reverse_reference': String that should be equal to another feature
+#                option. If set, it verifies that the feature referenced by
+#                |feature_reference| value references this feature using
+#                |reverse_reference| option.
 # If a type definition does not have any restrictions (beyond the type itself),
 # an empty definition ({}) is used.
 FEATURE_GRAMMAR = (
   {
+    'alias': {
+      unicode: {},
+      'feature_reference': {
+        'reverse_reference': 'source'
+      },
+      'shared': True
+    },
     'blacklist': {
       list: {'subtype': unicode}
     },
@@ -197,6 +215,13 @@ FEATURE_GRAMMAR = (
         }
       }
     },
+    'source': {
+      unicode: {},
+      'feature_reference': {
+        'reverse_reference': 'alias'
+      },
+      'shared': True
+    },
     'whitelist': {
       list: {'subtype': unicode}
     },
@@ -228,13 +253,26 @@ VALIDATION = ({
      'ManifestFeatures must specify at least one extension type'),
     (partial(DoesNotHaveProperty, 'contexts'),
      'ManifestFeatures do not support contexts.'),
+    (partial(DoesNotHaveProperty, 'alias'),
+     'ManifestFeatures do not support alias.'),
+    (partial(DoesNotHaveProperty, 'source'),
+     'ManifestFeatures do not support source.'),
   ],
-  'BehaviorFeature': [],
+  'BehaviorFeature': [
+    (partial(DoesNotHaveProperty, 'alias'),
+     'BehaviorFeatures do not support alias.'),
+    (partial(DoesNotHaveProperty, 'source'),
+     'BehaviorFeatures do not support source.'),
+   ],
   'PermissionFeature': [
     (partial(HasProperty, 'extension_types'),
      'PermissionFeatures must specify at least one extension type'),
     (partial(DoesNotHaveProperty, 'contexts'),
      'PermissionFeatures do not support contexts.'),
+    (partial(DoesNotHaveProperty, 'alias'),
+     'PermissionFeatures do not support alias.'),
+    (partial(DoesNotHaveProperty, 'source'),
+     'PermissionFeatures do not support source.'),
   ],
 })
 
@@ -260,6 +298,8 @@ class Feature(object):
     self.has_parent = False
     self.errors = []
     self.feature_values = {}
+    self.shared_values = {}
+    self.feature_references = []
 
   def _GetType(self, value):
     """Returns the type of the given value. This can be different than type() if
@@ -288,7 +328,7 @@ class Feature(object):
                        (self.name, key, error))
 
   def _GetCheckedValue(self, key, expected_type, expected_values,
-                       enum_map, value):
+                       enum_map, feature_reference, value):
     """Returns a string to be used in the generated C++ code for a given key's
     python value, or None if the value is invalid. For example, if the python
     value is True, this returns 'true', for a string foo, this returns "foo",
@@ -300,6 +340,16 @@ class Feature(object):
                        value is allowed.
       enum_map: The map from python value -> cpp value for all allowed values,
                or None if no special mapping should be made.
+      feature_reference: Object set if the value should represent a feature

[Devlin, 2016/11/21 16:55:53]

(With the above, we can remove this and most "feature_reference" code so that we
don't need to have knowledge of it everywhere)

[tbarzic, 2016/11/21 23:49:47]

Done.

+          reference. If set, the value is expected to be string referencing
+          another feature in the feature file. While it will be checked that the
+          value is string, validation that the value references an existing
+          feature will have to be postponed until all features are parsed - the
+          info needed to verify the key value will be cached in
+          |self.feature_references|.
+          The object may have "reverse_reference" property set - in that case
+          the referenced feature should reference this feature by
+          "reverse_reference" property.
       value: The value to check.
     """
     valid = True
@@ -312,6 +362,10 @@ class Feature(object):
       self._AddKeyError(key, 'Illegal value: "%s"' % value)
       valid = False
 
+    if feature_reference and not t in [str, unicode]:
+      self._AddKeyError(key, 'Illegal value for feature reference %s' % value)
+      valid = False
+
     if not valid:
       return None
 
@@ -319,6 +373,11 @@ class Feature(object):
       return enum_map[value]
 
     if t in [str, unicode]:
+      if feature_reference:
+        self.feature_references.append({
+          'value': str(value),
+          'key': key,
+          'reverse_reference': feature_reference.get('reverse_reference')})
       return '"%s"' % str(value)
     if t is int:
       return str(value)
@@ -342,6 +401,10 @@ class Feature(object):
       v = []
       is_all = True
 
+    if 'shared' in grammar and key in self.shared_values:
+      self._AddKeyError(key, 'Key can be set at most once per feature.')
+      return
+
     value_type = self._GetType(v)
     if value_type not in grammar:
       self._AddKeyError(key, 'Illegal value: "%s"' % v)
@@ -363,6 +426,12 @@ class Feature(object):
     if value_type is list and 'subtype' in expected:
       expected_type = expected['subtype']
 
+    feature_reference = None
+    if 'feature_reference' in grammar:
+      feature_reference = {}
+      feature_reference['reverse_reference'] = grammar['feature_reference'].get(
+          'reverse_reference')
+
     cpp_value = None
     # If this value is a list, iterate over each entry and validate. Otherwise,
     # validate the single value.
@@ -371,6 +440,7 @@ class Feature(object):
       for sub_value in v:
         cpp_sub_value = self._GetCheckedValue(key, expected_type,
                                               expected_values, enum_map,
+                                              feature_reference,
                                               sub_value)
         if cpp_sub_value:
           cpp_value.append(cpp_sub_value)
@@ -378,10 +448,13 @@ class Feature(object):
         cpp_value = '{' + ','.join(cpp_value) + '}'
     else:
       cpp_value = self._GetCheckedValue(key, expected_type, expected_values,
-                                        enum_map, v)
+                                        enum_map, feature_reference, v)
 
     if cpp_value:
-      self.feature_values[key] = cpp_value
+      if 'shared' in grammar:
+        self.shared_values[key] = cpp_value
+      else:
+        self.feature_values[key] = cpp_value
     elif key in self.feature_values:
       # If the key is empty and this feature inherited a value from its parent,
       # remove the inherited value.
@@ -395,6 +468,9 @@ class Feature(object):
     self.feature_values = copy.deepcopy(parent.feature_values)
     self.has_parent = True
 
+  def SetSharedValues(self, values):
+    self.shared_values = values
+
   def Parse(self, parsed_json):
     """Parses the feature from the given json value."""
     for key in parsed_json.keys():
@@ -404,21 +480,111 @@ class Feature(object):
       self._ParseKey(key, parsed_json, key_grammar)
 
   def Validate(self, feature_class):
+    feature_values = self.GetAllFeatureValues()
     for validator, error in (VALIDATION[feature_class] + VALIDATION['all']):
-      if not validator(self.feature_values):
+      if not validator(feature_values):
         self._AddError(error)
 
+  def _GetCodeForFeatureValues(self, feature_values):
+    """ Gets the Code object for setting feature values for this object. """
+    c = Code()
+    for key in sorted(feature_values.keys()):
+      if key in IGNORED_KEYS:
+        continue;
+      c.Append('feature->set_%s(%s);' % (key, feature_values[key]))
+    return c
+
   def GetCode(self, feature_class):
     """Returns the Code object for generating this feature."""
     c = Code()
     c.Append('%s* feature = new %s();' % (feature_class, feature_class))
     c.Append('feature->set_name("%s");' % self.name)
-    for key in sorted(self.feature_values.keys()):
-      if key in IGNORED_KEYS:
-        continue;
-      c.Append('feature->set_%s(%s);' % (key, self.feature_values[key]))
+    c.Concat(self._GetCodeForFeatureValues(self.GetAllFeatureValues()))
     return c
 
+  def AsParent(self):
+    """ Returns the feature values that should be inherited by children features
+    when this feature is set as parent.
+    """
+    return self
+
+  def GetAllFeatureValues(self):
+    """ Gets all values set for this feature. """
+    values = self.feature_values.copy()
+    values.update(self.shared_values)
+    return values
+
+  def ValidateFeatureReferences(self, features):
+    """ Validates feature values that reference another feature - the validation
+    cannot be done when individual features are compiled (like it is done for
+    other value types), as whole feature set is not know at that point.
+    """
+    for ref in self.feature_references:
+      # Validate that a feature referenced by this feature exists.
+      if not ref['value'] in features:
+        self._AddKeyError(ref['key'], '%s is not a feature.' % ref['value'])
+        return
+
+      # Verify that referenced feature references this feature, if reverse
+      # reference is required.
+      reverse_ref = ref['reverse_reference']
+      if reverse_ref:
+        referenced_feature_values = features[ref['value']].GetAllFeatureValues()
+        reverse_ref_value = referenced_feature_values.get(reverse_ref)
+        if (reverse_ref_value != ('"%s"' % self.name)):
+          self._AddKeyError(ref['key'],
+              'Referenced feature ("%s") should have "%s" set to "%s".' %
+                  (ref['value'], reverse_ref, self.name))
+
+  def GetErrors(self):
+    return self.errors;
+
+class ComplexFeature(Feature):
+  """ Complex feature - feature that is comprised of list of features.
+  Overall complex feature is available if any of contained
+  feature is available.
+  """
+  def __init__(self, name, shared_values):

[Devlin, 2016/11/21 16:55:53]

self.shared_values should start empty, so no need to pass in

[tbarzic, 2016/11/21 23:49:47]

Done.

+    Feature.__init__(self, name)
+    self.shared_values = shared_values
+    self.feature_list = []
+
+  def GetCode(self, feature_class):
+    c = Code()
+    c.Append('std::vector<Feature*> features;')
+    for f in self.feature_list:
+      c.Sblock('{')
+      c.Concat(f.GetCode(feature_class))
+      c.Append('features.push_back(feature);')
+      c.Eblock('}')
+    c.Append('ComplexFeature* feature(new ComplexFeature(&features));')
+    c.Append('feature->set_name("%s");' % self.name)
+    c.Concat(self._GetCodeForFeatureValues(self.GetAllFeatureValues()))
+    return c
+
+  def AsParent(self):
+    for p in self.feature_list:
+      if 'default_parent' in p.feature_values:
+        parent = p
+        break
+    assert parent, 'No default parent found for %s' % self.name
+    return parent
+
+  def ValidateFeatureReferences(self, available_features):
+    for feature in self.feature_list:
+      feature.ValidateFeatureReferences(available_features)
+
+  def GetAllFeatureValues(self):
+    return self.shared_values.copy()
+
+  def GetErrors(self):
+    errors = None
+    for feature in self.feature_list:
+      if not errors:
+        errors = []
+      errors.extend(feature.GetErrors())
+    return errors
+
 class FeatureCompiler(object):
   """A compiler to load, parse, and generate C++ code for a number of
   features.json files."""
@@ -486,15 +652,7 @@ class FeatureCompiler(object):
       #   raise KeyError('Could not find parent "%s" for feature "%s".' %
       #                      (parent_name, feature_name))
       return None
-    parent_value = self._features[parent_name]
-    parent = parent_value
-    if type(parent_value) is list:
-      for p in parent_value:
-        if 'default_parent' in p.feature_values:
-          parent = p
-          break
-      assert parent, 'No default parent found for %s' % parent_name
-    return parent
+    return self._features[parent_name].AsParent()
 
   def _CompileFeature(self, feature_name, feature_value):
     """Parses a single feature."""
@@ -503,11 +661,12 @@ class FeatureCompiler(object):
           'nocompile should only be true; otherwise omit this key.')
       return
 
-    def parse_and_validate(name, value, parent):
+    def parse_and_validate(name, value, parent, shared_values):
       try:
         feature = Feature(name)
         if parent:
           feature.SetParent(parent)
+        feature.SetSharedValues(shared_values)

[Devlin, 2016/11/21 16:55:53]

Ideally, this should be handled as part of feature.SetParent

[tbarzic, 2016/11/21 23:49:47]

parent might not be set

         feature.Parse(value)
         feature.Validate(self._feature_class)
         return feature
@@ -516,18 +675,21 @@ class FeatureCompiler(object):
         raise
 
     parent = self._FindParent(feature_name, feature_value)
+    shared_values = copy.deepcopy(parent.shared_values) if parent else {}
+
     # Handle complex features, which are lists of simple features.
     if type(feature_value) is list:
-      feature_list = []
+      feature = ComplexFeature(feature_name, shared_values)
+
       # This doesn't handle nested complex features. I think that's probably for
       # the best.
       for v in feature_value:
-        feature_list.append(parse_and_validate(feature_name, v, parent))
-      self._features[feature_name] = feature_list
-      return
-
-    self._features[feature_name] = parse_and_validate(
-                                       feature_name, feature_value, parent)
+        feature.feature_list.append(
+            parse_and_validate(feature_name, v, parent, feature.shared_values))
+      self._features[feature_name] = feature
+    else:
+      self._features[feature_name] = parse_and_validate(
+          feature_name, feature_value, parent, shared_values)
 
   def Compile(self):
     """Parses all features after loading the input files."""
@@ -535,6 +697,9 @@ class FeatureCompiler(object):
     # Iterate over in sorted order so that parents come first.
     for k in sorted(self._json.keys()):
       self._CompileFeature(k, self._json[k])
+    # Validate values that reference feature

[Devlin, 2016/11/21 16:55:53]

Then here

def _FinalValidation(self):
  for name, feature in self._features.items():
     for validator, error in (FINAL_VALIDATION[self._feature_class] +
FINAL_VALIDATION['all']):
       if not validator(self._features, feature):
         self._AddError(error)

def Compile(self):
  ....
  self._FinalValidation()

[tbarzic, 2016/11/21 23:49:47]

Done.

+    for key in self._features:
+      self._features[key].ValidateFeatureReferences(self._features)
 
   def Render(self):
     """Returns the Code object for the body of the .cc file, which handles the
@@ -545,17 +710,7 @@ class FeatureCompiler(object):
     for k in sorted(self._features.keys()):
       c.Sblock('{')
       feature = self._features[k]
-      if type(feature) is list:
-        c.Append('std::vector<Feature*> features;')
-        for f in feature:
-          c.Sblock('{')
-          c.Concat(f.GetCode(self._feature_class))
-          c.Append('features.push_back(feature);')
-          c.Eblock('}')
-        c.Append('ComplexFeature* feature(new ComplexFeature(&features));')
-        c.Append('feature->set_name("%s");' % k)
-      else:
-        c.Concat(feature.GetCode(self._feature_class))
+      c.Concat(feature.GetCode(self._feature_class))
       c.Append('AddFeature("%s", feature);' % k)
       c.Eblock('}')
     c.Eblock('}')