Validation doesn't fail using custom errors of type `Error`

[Giners]

Per the Joi documentation one may provide custom errors (documentation: https://github.com/hapijs/joi/blob/v10.6.0/API.md#anyerrorerr). These custom errors override the default Joi error. Here is a snippet producing the custom error (taken from documentation):

const schema = Joi.string().error(new Error('Was REALLY expecting a string'));
schema.validate(3);     // returns error.message === 'Was REALLY expecting a string'

Joi validation will fail as expected and produce a custom error of type Error yet dynogels validation doesn't fail as expected. For my use case (updating an item) this is the offending piece of code:

https://github.com/clarkie/dynogels/blob/6b3f75c123ec1924d70d0ab1178519012c98b079/lib/table.js#L274

internals.validateItemFragment = (item, schema) => {
  // ...

  if (updateValidation.error) {
    const errors = _.omitBy(
      updateValidation.error.details,
      e => _.isEqual(e.type, 'any.required')
    );
    if (!_.isEmpty(errors)) {
      error.update = errors;
      result.error = error;
    }
  }

  return result;
};

dynogels validation doesn't fail as expected as the custom error of type Error doesn't have the details property, thus nothing gets checked by the predicate in the _.omitBy(...) call. This is also documented in the Joi documentation.

It would seem that one would also want validation to fail in the event custom errors of type Error are used. I didn't see this documented anywhere but maybe I missed something that clearly says don't do this?

Note that while I provided a specific use case and the offending code it is plausible that this pattern exists in other places where validation occurs in Dynogels and we should see it break as well.

[jkav77]

So just to make sure I understand, you can declare custom error types when you define the schema, right? But if you use custom Errors the validation doesn't fail, even if the field fails to validate and throws the error. Let me know if I am off track here...

[jkav77]

@Giners does #100 relate to this issue at all?

[Giners]

@dangerginger You said:

So just to make sure I understand, you can declare custom error types when you define the schema, right?

This is correct. You also said:

But if you use custom Errors the validation doesn't fail, even if the field fails to validate and throws the error.

This is incorrect (it seems my original wording wasn't as clear as it could have been and I have updated it since to make it more clear). Joi validation correctly fails and a custom error of type Error is produced. Yet dynogels validation (i.e. validateItemFragment()) doesn't seem to validate correctly.

100/#73 doesn't seem related. As I understand #100/#73 they are more about adding more informative/specific errors but in this case a more informative/specific error wouldn't be produced.

[jkav77]

Ok. I'm willing to take a look at it. Can you write a test or give an example?

[Giners]

@dangerginger Sorry it took so long to get back to you. This is pretty much what I have in my project/tests. Give something like this a try and let me know if you are able to reproduce it...

// UserModel.js

import Joi from 'joi'
import dynogels from './dynogels'

export const UserModel = dynogels.define('User', {
  hashKey: 'email',
  timestamps: true,
  schema: {
    email: Joi.string().email({ errorLevel: true }).required(),
    password: Joi.any().forbidden().error(new Error('Only hashed passwords should be persisted')),
    passwordHash: Joi.string().required(),
  },
})

// UserModel.test.js

import chai, { expect } from 'chai'
import chaiAsPromised from 'chai-as-promised'

// Code under test
import User from './User'

// Configure Chai usage to work with Promises
chai.use(chaiAsPromised)

describe('DB User model test:', function () {
  before('Setup the DB (user table)', async function () {
    // Do some setup as needed here...
  })

  describe('update():', function () {
    it('Promise is rejected if password property set', async function () {
      const user = await User.create({ email: 'fruits@test.com', passwordHash: 'HashedPassword' })

      // Setting the password property ought to cause the promise returned by 'User.update()' to be rejected...
      user.password = 'Bananas'

      const promise = User.update(user)

      // Validate that the promise is rejected...
      return expect(promise).to.be.rejectedWith('Only hashed passwords should be persisted')
    })
  })
})

Quick note: I wrap the dynogels callbacks in promises.

This test ought to pass as the promise is rejected when I provide the forbidden property password when trying to update but the test fails as the promises resolves an updated user (i.e. validation in dynogels doesn't fail even though Joi validation did).

If you change the password property on the schema to override the key name in the error with the label() method vs providing an error you ought to see the test pass as the promise is rejected.

password: Joi.any().forbidden().label('Only hashed passwords should be persisted'),

[Giners]

How goes it @dangerginger? Any visibility into your progress?

[jkav77]

@Giners I have been pretty busy the last couple weeks. I started looking at it the other day and will probably finish it off shortly.

[Giners]

@dangerginger No worries. Ping me if you need any more info/insights but it sounds like you got this. 😄

If you need any reviewers you can add me as well to your PRs.

[jkav77]

This is a bit complicated because Joi doesn't provide any way to partially validate a schema. The schema you provide to create a dynogels model is for a complete model but when you do an update you only provide the attributes you want to update. In table.js there is a function called internals.validateItemFragment that attempts to ensure that a partial update on an item doesn't end up breaking the schema. It is kind of a mess because it is hackishly implementing a Joi feature that doesn't exist.

Dynogels create() still manages to throw custom errors because it can just use the Job schema.validate method.

I got update() working now for updating attributes, but if you remove an attribute that is required() and that has a custom error() specified in the Joi schema then it will fail to update but return no error at all.

[jkav77]

I think #127 fixes this. Check it out.

[jkav77]

@Giners I think the answer to this is that the custom errors should work for create requests because this uses Joi to validate the schema of an entire model. The reason it didn't work for updaterequests was because Joi does not have any way to validate only part of a schema. I think solutions for the problem of validating an update might be to create a Joi schema that describes a valid update.