Bug 1225715: Part 4 - Improve reporting of schema errors. r?billm draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 22 Jan 2016 17:14:54 -0800
changeset 324469 bda062baae5de13fd0fb6cd608580c95ddc2c991
parent 324463 4905fa3a603e1cd4c4e9ca98d2dfcffc7fa103f3
child 324470 a0978eeeac09d88a0e6505d71f68896eaefd0fb9
push id9922
push usermaglione.k@gmail.com
push dateSat, 23 Jan 2016 01:23:37 +0000
reviewersbillm
bugs1225715
milestone46.0a1
Bug 1225715: Part 4 - Improve reporting of schema errors. r?billm
toolkit/components/extensions/Schemas.jsm
toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -71,16 +71,58 @@ function getValueBaseType(value) {
   } else if (t == "number") {
     if (value % 1 == 0) {
       return "integer";
     }
   }
   return t;
 }
 
+class Context {
+  constructor(params) {
+    this.params = params;
+
+    this.path = [];
+
+    let props = ["addListener", "callFunction", "checkLoadURL",
+                 "hasListener", "removeListener"];
+    for (let prop of props) {
+      this[prop] = params[prop];
+    }
+  }
+
+  get url() {
+    return this.params.url;
+  }
+
+  get cloneScope() {
+    return this.params.cloneScope;
+  }
+
+  error(message) {
+    if (this.currentTarget) {
+      return {error: `Error processing ${this.currentTarget}: ${message}`};
+    }
+    return {error: message};
+  }
+
+  get currentTarget() {
+    return this.path.join(".");
+  }
+
+  withPath(component, callback) {
+    this.path.push(component);
+    try {
+      return callback();
+    } finally {
+      this.path.pop();
+    }
+  }
+}
+
 const FORMATS = {
   url(string, context) {
     let url = new URL(string).href;
 
     context.checkLoadURL(url);
     return url;
   },
 
@@ -122,36 +164,36 @@ class Entry {
 // Corresponds either to a type declared in the "types" section of the
 // schema or else to any type object used throughout the schema.
 class Type extends Entry {
   // Takes a value, checks that it has the correct type, and returns a
   // "normalized" version of the value. The normalized version will
   // include "nulls" in place of omitted optional properties. The
   // result of this function is either {error: "Some type error"} or
   // {value: <normalized-value>}.
-  normalize(value) {
-    return {error: "invalid type"};
+  normalize(value, context) {
+    return context.error("invalid type");
   }
 
   // Unlike normalize, this function does a shallow check to see if
   // |baseType| (one of the possible getValueBaseType results) is
   // valid for this type. It returns true or false. It's used to fill
   // in optional arguments to functions before actually type checking
   // the arguments.
   checkBaseType(baseType) {
     return false;
   }
 
   // Helper method that simply relies on checkBaseType to implement
   // normalize. Subclasses can choose to use it or not.
-  normalizeBase(type, value) {
+  normalizeBase(type, value, context) {
     if (this.checkBaseType(getValueBaseType(value))) {
       return {value};
     }
-    return {error: `Expected ${type} instead of ${JSON.stringify(value)}`};
+    return context.error(`Expected ${type} instead of ${JSON.stringify(value)}`);
   }
 }
 
 // Type that allows any value.
 class AnyType extends Type {
   normalize(value) {
     return {value};
   }
@@ -170,24 +212,30 @@ class ChoiceType extends Type {
 
   extend(type) {
     this.choices.push(...type.choices);
 
     return this;
   }
 
   normalize(value, context) {
+    let error;
+
+    let baseType = getValueBaseType(value);
     for (let choice of this.choices) {
-      let r = choice.normalize(value, context);
-      if (!r.error) {
-        return r;
+      if (choice.checkBaseType(baseType)) {
+        let r = choice.normalize(value, context);
+        if (!r.error) {
+          return r;
+        }
+        error = r.error;
       }
     }
 
-    return {error: "No valid choice"};
+    return context.error(error || `Unexpected value ${JSON.stringify(value)}`);
   }
 
   checkBaseType(baseType) {
     return this.choices.some(t => t.checkBaseType(baseType));
   }
 }
 
 // This is a reference to another type--essentially a typedef.
@@ -224,44 +272,44 @@ class StringType extends Type {
     this.enumeration = enumeration;
     this.minLength = minLength;
     this.maxLength = maxLength;
     this.pattern = pattern;
     this.format = format;
   }
 
   normalize(value, context) {
-    let r = this.normalizeBase("string", value);
+    let r = this.normalizeBase("string", value, context);
     if (r.error) {
       return r;
     }
 
     if (this.enumeration) {
       if (this.enumeration.includes(value)) {
         return {value};
       }
-      return {error: `Invalid enumeration value ${JSON.stringify(value)}`};
+      return context.error(`Invalid enumeration value ${JSON.stringify(value)}`);
     }
 
     if (value.length < this.minLength) {
-      return {error: `String ${JSON.stringify(value)} is too short (must be ${this.minLength})`};
+      return context.error(`String ${JSON.stringify(value)} is too short (must be ${this.minLength})`);
     }
     if (value.length > this.maxLength) {
-      return {error: `String ${JSON.stringify(value)} is too long (must be ${this.maxLength})`};
+      return context.error(`String ${JSON.stringify(value)} is too long (must be ${this.maxLength})`);
     }
 
     if (this.pattern && !this.pattern.test(value)) {
-      return {error: `String ${JSON.stringify(value)} must match ${this.pattern}`};
+      return context.error(`String ${JSON.stringify(value)} must match ${this.pattern}`);
     }
 
     if (this.format) {
       try {
         r.value = this.format(r.value, context);
       } catch (e) {
-        return {error: String(e)};
+        return context.error(String(e));
       }
     }
 
     return r;
   }
 
   checkBaseType(baseType) {
     return baseType == "string";
@@ -300,30 +348,30 @@ class ObjectType extends Type {
     return this;
   }
 
   checkBaseType(baseType) {
     return baseType == "object";
   }
 
   normalize(value, context) {
-    let v = this.normalizeBase("object", value);
+    let v = this.normalizeBase("object", value, context);
     if (v.error) {
       return v;
     }
 
     if (this.isInstanceOf) {
       if (Object.keys(this.properties).length ||
           this.patternProperties.length ||
           !(this.additionalProperties instanceof AnyType)) {
         throw new Error("InternalError: isInstanceOf can only be used with objects that are otherwise unrestricted");
       }
 
       if (!instanceOf(value, this.isInstanceOf)) {
-        return {error: `Object must be an instance of ${this.isInstanceOf}`};
+        return context.error(`Object must be an instance of ${this.isInstanceOf}`);
       }
 
       // This is kind of a hack, but we can't normalize things that
       // aren't JSON, so we just return them.
       return {value};
     }
 
     // |value| should be a JS Xray wrapping an object in the
@@ -331,56 +379,56 @@ class ObjectType extends Type {
     // access callable properties on |value| since JS Xrays don't
     // support those. To work around the problem, we verify that
     // |value| is a plain JS object (i.e., not anything scary like a
     // Proxy). Then we copy the properties out of it into a normal
     // object using a waiver wrapper.
 
     let klass = Cu.getClassName(value, true);
     if (klass != "Object") {
-      return {error: `Expected a plain JavaScript object, got a ${klass}`};
+      return context.error(`Expected a plain JavaScript object, got a ${klass}`);
     }
 
     let properties = Object.create(null);
     {
       // |waived| is scoped locally to avoid accessing it when we
       // don't mean to.
       let waived = Cu.waiveXrays(value);
       for (let prop of Object.getOwnPropertyNames(waived)) {
         let desc = Object.getOwnPropertyDescriptor(waived, prop);
         if (desc.get || desc.set) {
-          return {error: "Objects cannot have getters or setters on properties"};
+          return context.error("Objects cannot have getters or setters on properties");
         }
         if (!desc.enumerable) {
           // Chrome ignores non-enumerable properties.
           continue;
         }
         properties[prop] = Cu.unwaiveXrays(desc.value);
       }
     }
 
     let result = {};
     let checkProperty = (prop, propType) => {
       let {type, optional, unsupported} = propType;
       if (unsupported) {
         if (prop in properties) {
-          return {error: `Property "${prop}" is unsupported by Firefox`};
+          return context.error(`Property "${prop}" is unsupported by Firefox`);
         }
       } else if (prop in properties) {
         if (optional && (properties[prop] === null || properties[prop] === undefined)) {
           result[prop] = null;
         } else {
-          let r = type.normalize(properties[prop], context);
+          let r = context.withPath(prop, () => type.normalize(properties[prop], context));
           if (r.error) {
             return r;
           }
           result[prop] = r.value;
         }
       } else if (!optional) {
-        return {error: `Property "${prop}" is required`};
+        return context.error(`Property "${prop}" is required`);
       } else {
         result[prop] = null;
       }
     };
 
     for (let prop of Object.keys(this.properties)) {
       let error = checkProperty(prop, this.properties[prop]);
       if (error) {
@@ -401,39 +449,40 @@ class ObjectType extends Type {
             return error;
           }
 
           continue outer;
         }
       }
 
       if (this.additionalProperties) {
-        let r = this.additionalProperties.normalize(properties[prop], context);
+        let type = this.additionalProperties;
+        let r = context.withPath(prop, () => type.normalize(properties[prop], context));
         if (r.error) {
           return r;
         }
         result[prop] = r.value;
       } else {
-        return {error: `Unexpected property "${prop}"`};
+        return context.error(`Unexpected property "${prop}"`);
       }
     }
 
     return {value: result};
   }
 }
 
 class NumberType extends Type {
-  normalize(value) {
-    let r = this.normalizeBase("number", value);
+  normalize(value, context) {
+    let r = this.normalizeBase("number", value, context);
     if (r.error) {
       return r;
     }
 
     if (isNaN(value) || !Number.isFinite(value)) {
-      return {error: "NaN or infinity are not valid"};
+      return context.error("NaN or infinity are not valid");
     }
 
     return r;
   }
 
   checkBaseType(baseType) {
     return baseType == "number" || baseType == "integer";
   }
@@ -441,99 +490,99 @@ class NumberType extends Type {
 
 class IntegerType extends Type {
   constructor(minimum, maximum) {
     super();
     this.minimum = minimum;
     this.maximum = maximum;
   }
 
-  normalize(value) {
-    let r = this.normalizeBase("integer", value);
+  normalize(value, context) {
+    let r = this.normalizeBase("integer", value, context);
     if (r.error) {
       return r;
     }
 
     // Ensure it's between -2**31 and 2**31-1
     if ((value | 0) !== value) {
-      return {error: "Integer is out of range"};
+      return context.error("Integer is out of range");
     }
 
     if (value < this.minimum) {
-      return {error: `Integer ${value} is too small (must be at least ${this.minimum})`};
+      return context.error(`Integer ${value} is too small (must be at least ${this.minimum})`);
     }
     if (value > this.maximum) {
-      return {error: `Integer ${value} is too big (must be at most ${this.maximum})`};
+      return context.error(`Integer ${value} is too big (must be at most ${this.maximum})`);
     }
 
     return r;
   }
 
   checkBaseType(baseType) {
     return baseType == "integer";
   }
 }
 
 class BooleanType extends Type {
-  normalize(value) {
-    return this.normalizeBase("boolean", value);
+  normalize(value, context) {
+    return this.normalizeBase("boolean", value, context);
   }
 
   checkBaseType(baseType) {
     return baseType == "boolean";
   }
 }
 
 class ArrayType extends Type {
   constructor(itemType, minItems, maxItems) {
     super();
     this.itemType = itemType;
     this.minItems = minItems;
     this.maxItems = maxItems;
   }
 
   normalize(value, context) {
-    let v = this.normalizeBase("array", value);
+    let v = this.normalizeBase("array", value, context);
     if (v.error) {
       return v;
     }
 
     let result = [];
-    for (let element of value) {
-      element = this.itemType.normalize(element, context);
+    for (let [i, element] of value.entries()) {
+      element = context.withPath(i, () => this.itemType.normalize(element, context));
       if (element.error) {
         return element;
       }
       result.push(element.value);
     }
 
     if (result.length < this.minItems) {
-      return {error: `Array requires at least ${this.minItems} items; you have ${result.length}`};
+      return context.error(`Array requires at least ${this.minItems} items; you have ${result.length}`);
     }
 
     if (result.length > this.maxItems) {
-      return {error: `Array requires at most ${this.maxItems} items; you have ${result.length}`};
+      return context.error(`Array requires at most ${this.maxItems} items; you have ${result.length}`);
     }
 
     return {value: result};
   }
 
   checkBaseType(baseType) {
     return baseType == "array";
   }
 }
 
 class FunctionType extends Type {
   constructor(parameters) {
     super();
     this.parameters = parameters;
   }
 
-  normalize(value) {
-    return this.normalizeBase("function", value);
+  normalize(value, context) {
+    return this.normalizeBase("function", value, context);
   }
 
   checkBaseType(baseType) {
     return baseType == "function";
   }
 }
 
 // Represents a "property" defined in a schema namespace with a
@@ -655,57 +704,59 @@ class FunctionEntry extends CallEntry {
   }
 
   inject(name, dest, wrapperFuncs) {
     if (this.unsupported) {
       return;
     }
 
     let stub = (...args) => {
-      let actuals = this.checkParameters(args, dest, wrapperFuncs);
+      let actuals = this.checkParameters(args, dest, new Context(wrapperFuncs));
       return wrapperFuncs.callFunction(this.namespaceName, name, actuals);
     };
     Cu.exportFunction(stub, dest, {defineAs: name});
   }
 }
 
 // Represents an "event" defined in a schema namespace.
 class Event extends CallEntry {
   constructor(namespaceName, name, type, extraParameters, unsupported) {
     super(namespaceName, name, extraParameters);
     this.type = type;
     this.unsupported = unsupported;
   }
 
-  checkListener(global, listener) {
-    let r = this.type.normalize(listener);
+  checkListener(global, listener, context) {
+    let r = this.type.normalize(listener, context);
     if (r.error) {
       this.throwError(global, "Invalid listener");
     }
     return r.value;
   }
 
   inject(name, dest, wrapperFuncs) {
     if (this.unsupported) {
       return;
     }
 
+    let context = new Context(wrapperFuncs);
+
     let addStub = (listener, ...args) => {
-      listener = this.checkListener(dest, listener);
-      let actuals = this.checkParameters(args, dest);
+      listener = this.checkListener(dest, listener, context);
+      let actuals = this.checkParameters(args, dest, context);
       return wrapperFuncs.addListener(this.namespaceName, name, listener, actuals);
     };
 
     let removeStub = (listener) => {
-      listener = this.checkListener(dest, listener);
+      listener = this.checkListener(dest, listener, context);
       return wrapperFuncs.removeListener(this.namespaceName, name, listener);
     };
 
     let hasStub = (listener) => {
-      listener = this.checkListener(dest, listener);
+      listener = this.checkListener(dest, listener, context);
       return wrapperFuncs.hasListener(this.namespaceName, name, listener);
     };
 
     let obj = Cu.createObjectIn(dest, {defineAs: name});
     Cu.exportFunction(addStub, obj, {defineAs: "addListener"});
     Cu.exportFunction(removeStub, obj, {defineAs: "removeListener"});
     Cu.exportFunction(hasStub, obj, {defineAs: "hasListener"});
   }
@@ -728,17 +779,17 @@ this.Schemas = {
   parseType(namespaceName, type, extraProperties = []) {
     let allowedProperties = new Set(extraProperties);
 
     // Do some simple validation of our own schemas.
     function checkTypeProperties(...extra) {
       let allowedSet = new Set([...allowedProperties, ...extra, "description"]);
       for (let prop of Object.keys(type)) {
         if (!allowedSet.has(prop)) {
-          throw new Error(`Internal error: Namespace ${namespaceName} has invalid type property "${prop}" in type "${type.name}"`);
+          throw new Error(`Internal error: Namespace ${namespaceName} has invalid type property "${prop}" in type "${type.id || JSON.stringify(type)}"`);
         }
       }
     }
 
     if ("choices" in type) {
       checkTypeProperties("choices");
 
       let choices = type.choices.map(t => this.parseType(namespaceName, t));
@@ -982,13 +1033,13 @@ this.Schemas = {
       }
     });
   },
 
   inject(dest, wrapperFuncs) {
     for (let [namespace, ns] of this.namespaces) {
       let obj = Cu.createObjectIn(dest, {defineAs: namespace});
       for (let [name, entry] of ns) {
-        entry.inject(name, obj, wrapperFuncs);
+        entry.inject(name, obj, new Context(wrapperFuncs));
       }
     }
   },
 };
--- a/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
@@ -1,10 +1,9 @@
-"use strict";
-
+https://reviewboard.mozilla.org/r/32037
 Components.utils.import("resource://gre/modules/Schemas.jsm");
 Components.utils.import("resource://gre/modules/BrowserUtils.jsm");
 
 let json = [
   {namespace: "testing",
 
    properties: {
      PROP1: {value: 20},
@@ -192,16 +191,49 @@ let json = [
              relativeUrl: {type: "string", "format": "relativeUrl", "optional": true},
              strictRelativeUrl: {type: "string", "format": "strictRelativeUrl", "optional": true},
            },
          },
        ],
      },
 
      {
+       name: "deep",
+       type: "function",
+       parameters: [
+         {
+           name: "arg",
+           type: "object",
+           properties: {
+             foo: {
+               type: "object",
+               properties: {
+                 bar: {
+                   type: "array",
+                   items: {
+                     type: "object",
+                     properties: {
+                       baz: {
+                         type: "object",
+                         properties: {
+                           required: {type: "integer"},
+                           optional: {type: "string", optional: true},
+                         },
+                       },
+                     },
+                   },
+                 },
+               },
+             },
+           },
+         },
+       ],
+     },
+
+     {
        name: "extended1",
        type: "function",
        parameters: [
          {name: "val", $ref: "basetype1"},
        ],
      },
 
      {
@@ -444,16 +476,28 @@ add_task(function* () {
   }
 
   for (let url of ["//foo.html", "http://foo/bar.html"]) {
     Assert.throws(() => root.testing.format({strictRelativeUrl: url}),
                   /must be a relative URL/,
                   "should throw for non-relative URL");
   }
 
+  root.testing.deep({foo: {bar: [{baz: {required: 12, optional: "42"}}]}});
+  verify("call", "testing", "deep", [{foo: {bar: [{baz: {required: 12, optional: "42"}}]}}]);
+  tallied = null;
+
+  Assert.throws(() => root.testing.deep({foo: {bar: [{baz: {optional: "42"}}]}}),
+                /Type error for parameter arg \(Error processing foo\.bar\.0\.baz: Property "required" is required\) for testing\.deep/,
+                "should throw with the correct object path");
+
+  Assert.throws(() => root.testing.deep({foo: {bar: [{baz: {required: 12, optional: 42}}]}}),
+                /Type error for parameter arg \(Error processing foo\.bar\.0\.baz\.optional: Expected string instead of 42\) for testing\.deep/,
+                "should throw with the correct object path");
+
   root.testing.onFoo.addListener(f);
   do_check_eq(JSON.stringify(tallied.slice(0, -1)), JSON.stringify(["addListener", "testing", "onFoo"]));
   do_check_eq(tallied[3][0], f);
   do_check_eq(JSON.stringify(tallied[3][1]), JSON.stringify([]));
   tallied = null;
 
   root.testing.onFoo.removeListener(f);
   do_check_eq(JSON.stringify(tallied.slice(0, -1)), JSON.stringify(["removeListener", "testing", "onFoo"]));