Repository: TechnologyAdvice/obey
Pull request #26 (2016-03-07 15:00:06)

Required prop fix
Fixes issue with `require`'d props by:

- Default `required` state `false`
- Only runs full props checks on values that exist (not `undefined`) and are not required

Closes #26 


Patch:
diff --git a/README.md b/README.md
index e9ceaba..ae71c69 100644
--- a/README.md
+++ b/README.md
@@ -132,7 +132,7 @@ When setting definitions for rules or model properties, the following are suppor
 * `default`: The default value if no value specified
 * `min`: The minimum character length for a string, lowest number, or minimum items in array
 * `max`: The maximum character length for a string, highest number, or maximum items in array
-* `required`: Enforces the field cannot be missing during validation
+* `required`: Enforces the field cannot be missing during validation (default `false`)
 * `allow`: Array of allowed values or single allowed value
 * `strict`: Enable or disable strict checking of an object, see [Strict Mode](#strict-mode)
 * `description`: A description of the property
diff --git a/src/lib/validators.js b/src/lib/validators.js
index 5f0bb3d..0cf9b33 100644
--- a/src/lib/validators.js
+++ b/src/lib/validators.js
@@ -59,18 +59,6 @@ const validators = {
     } else if (typeof value === 'number' && value > def.max) {
       this.errors.push({ type, sub, key, value, message: `Value must be less than ${def.max}` })
     }
-  },
-
-  /**
-   * Validator requires method, used by model
-   * @param {Object} def The property configuration
-   * @param {String} key The key name of the property
-   * @param {*} value The value being validated
-   */
-  required: function(def, key, value) {
-    if (def.required && !value) {
-      this.errors.push({ type: 'required', sub: 'default', key, value, message: `Property '${key}' is required` })
-    }
   }
 }
 
diff --git a/src/rules.js b/src/rules.js
index c133ff8..9f4b015 100644
--- a/src/rules.js
+++ b/src/rules.js
@@ -8,21 +8,42 @@ import Promise from 'bluebird'
 import validators from './lib/validators'
 import ValidationError from './lib/error'
 
+/**
+ * Defines all definition property checks available
+ */
+const allProps = {
+  generator: { name: 'generator', fn: generators.validator },
+  default: { name: 'default', fn: validators.default },
+  modifier: { name: 'modifier', fn: modifiers.validator },
+  allow: { name: 'allow', fn: validators.allow },
+  min: { name: 'min', fn: validators.min },
+  max: { name: 'max', fn: validators.max },
+  type: { name: 'type', fn: types.validator }
+}
+
 const rules = {
   /**
    * Acts as validation setup for, and respective order of operations
    * of, properties for a def-prop configuration
    */
-  props: [
-    { name: 'generator', fn: generators.validator },
-    { name: 'default', fn: validators.default },
-    { name: 'modifier', fn: modifiers.validator },
-    { name: 'allow', fn: validators.allow },
-    { name: 'min', fn: validators.min },
-    { name: 'max', fn: validators.max },
-    { name: 'type', fn: types.validator },
-    { name: 'required', fn: validators.required }
-  ],
+  props: {
+    // Default props
+    default: [
+      allProps.generator,
+      allProps.default,
+      allProps.modifier,
+      allProps.allow,
+      allProps.min,
+      allProps.max,
+      allProps.type
+    ],
+    // When no value/undefined
+    noVal: [
+      allProps.generator,
+      allProps.default,
+      allProps.modifier
+    ]
+  },
 
   /**
    * Binds rule definition in validate method
@@ -38,9 +59,10 @@ const rules = {
    */
   validate: (def, data, key = null) => {
     const context = { errors: [] }
+    const props = !def.required && data === undefined ? rules.props.noVal : rules.props.default
     if (!def.type) throw new Error('Model properties must define a \'type\'')
     let chain = Promise.resolve(data)
-    rules.props.forEach(prop => {
+    props.forEach(prop => {
       if (def[prop.name]) {
         chain = chain.then(prop.fn.bind(context, def, key)).then(res => {
           return res === undefined ? data : res


Test patch:
diff --git a/test/fixtures/core.js b/test/fixtures/core.js
index 5e4b178..6bb0f9a 100644
--- a/test/fixtures/core.js
+++ b/test/fixtures/core.js
@@ -5,9 +5,13 @@ export default {
   missingType: {
     name: { required: true }
   },
+  basicRequired: {
+    fname: { type: 'string' },
+    lname: { type: 'string', required: true }
+  },
   basicExtended: {
     fname: { type: 'string', required: true, min: 2, max: 20 },
-    lname: { type: 'string', required: true, min: 2, max: 20 },
+    lname: { type: 'string', min: 2, max: 20 },
     type: { type: 'string', allowed: [ 'foo', 'bar' ] }
   },
   basicNested: {
diff --git a/test/integration/core.spec.js b/test/integration/core.spec.js
index f2cbe32..ff63063 100644
--- a/test/integration/core.spec.js
+++ b/test/integration/core.spec.js
@@ -26,6 +26,29 @@ describe('integration:core', () => {
       expect(e).to.be.instanceOf(ValidationError)
     })
   })
+  it('builds a model and passes when non-required field is undefined', () => {
+    const testModel = obey.model(modelFixtures.basicExtended)
+    const testData = {
+      fname: 'John'
+    }
+    return testModel.validate(testData)
+      .then((res) => {
+        expect(res.fname).to.equal('John')
+        expect(res.lname).to.be.undefined
+        expect(res.type).to.be.undefined
+      })
+  })
+  it('builds a model and fails when required field is undefined', () => {
+    const testModel = obey.model(modelFixtures.basicRequired)
+    const testData = {
+      fname: 'John'
+    }
+    return testModel.validate(testData)
+      .then(() => { throw new Error('Should fail') })
+      .catch((err) => {
+        expect(err.message).to.equal('lname (undefined): Value must be a string')
+      })
+  })
   it('builds a model and successfully validates when nested object present', () => {
     const testModel = obey.model(modelFixtures.basicNested)
     const testData = {
diff --git a/test/integration/validators.spec.js b/test/integration/validators.spec.js
index 56ee5fc..8fd1318 100644
--- a/test/integration/validators.spec.js
+++ b/test/integration/validators.spec.js
@@ -57,25 +57,4 @@ describe('integration:validators', () => {
       })
     })
   })
-  describe('required', () => {
-    it('builds a model and fails validation because value is required', () => {
-      const testModel = obey.model(modelFixtures.required)
-      const testData = {}
-      return testModel.validate(testData).catch(err => {
-        expect(err.collection).to.deep.equal([{
-          type: 'string',
-          sub: 'default',
-          key: 'name',
-          value: undefined,
-          message: 'Value must be a string'
-        }, {
-          type: 'required',
-          sub: 'default',
-          key: 'name',
-          value: undefined,
-          message: 'Property \'name\' is required'
-        }])
-      })
-    })
-  })
 })
diff --git a/test/src/lib/validators.spec.js b/test/src/lib/validators.spec.js
index e15ffe1..02cf735 100644
--- a/test/src/lib/validators.spec.js
+++ b/test/src/lib/validators.spec.js
@@ -134,17 +134,4 @@ describe('validators', () => {
       })
     })
   })
-  describe('required', () => {
-    it('creates an error object if a required value is not supplied', () => {
-      const def = { required: true }
-      validators.required.call(mockThis, def, 'test', undefined)
-      expect(mockThis.errors[0]).to.deep.equal({
-        type: 'required',
-        sub: 'default',
-        key: 'test',
-        value: undefined,
-        message: 'Property \'test\' is required'
-      })
-    })
-  })
 })