Bug with using Joi.object() for route validation

[saitheexplorer]

I think I've encountered some unexpected behavior with how hapi and joi work together to validate a route. What I'm seeing could be two separate bugs, or side effects of the same bug. included some minimal test scenarios as well.

Scenario 1:

When initializing the server, I want to provide some default validation that will be present on every route. In server.connection, I pass my schema as a POJO wrapper around Joi rules: e.g.:

server.connection({
  routes: {
    validate: {
      query: {
        api_key: Joi.string().required()
      }
    }
})

In one of my routes, I want to use Joi to say one of two field must be present in the query, e.g. email or user_id. My route looks as follows:

server.route({
  path: '/test',
  method: 'GET',
  handler: (req, reply) => reply('success'),
  config: {
    validate: {
      query: Joi.object().keys({
        email: Joi.string(),
        user_id: Joi.string()
      }).or('email', 'user_id')
    }
  }
});

If I hit /test, the route hangs and hapi emits an error:

TypeError: schema._validateWithOptions is not a function
at root.validate (PROJECT/node_modules/hapi/node_modules/joi/lib/index.js:102:23)
at Object.internals.input.postValidate [as input] (PROJECT/node_modules/hapi/lib/validation.js:128:20)
at exports.query (PROJECT/node_modules/hapi/lib/validation.js:17:22)
at each (PROJECT/node_modules/hapi/lib/request.js:383:16)
at iterate (PROJECT/node_modules/hapi/node_modules/items/lib/index.js:36:13)
at done (PROJECT/node_modules/hapi/node_modules/items/lib/index.js:28:25)
at internals.Auth.test.internals.Auth._authenticate (PROJECT/node_modules/hapi/lib/auth.js:210:16)
at internals.Auth.test.internals.Auth.authenticate (PROJECT/node_modules/hapi/lib/auth.js:202:17)
at each (PROJECT/node_modules/hapi/lib/request.js:383:16)
at iterate (PROJECT/node_modules/hapi/node_modules/items/lib/index.js:36:13)

This can be solved if I change the default joi route validation to use Joi.object() and not a POJO. However, this behavior could be improved - even if mixing Joi.object()/POJOs is not supported, can that be checked when the route is registered rather than when the route is requested?

Here is a gist with the bug reproduced with latest hapi & joi:
https://gist.github.com/saitheexplorer/e7b108895e37d86857f3

Scenario 2:

With the fix for scenario 1 in place (default validation is a Joi object), I add another route (/another-test) - this one does not require any validation other than the connection defaults.

server.route({
  path: '/another-test',
  method: 'GET',
  handler: (req, reply) => reply('success'),
});

Request to /test without query (the original route with required query params) fails as expected. Requesting /another-test without query, however, also fails - it says email or user_id are required, despite having no validation for that route.

If I change the server connection default validation back to a POJO, my second route replies fine with no validation errors, but because of Scenario 1, the first route now hangs again.

Gist with second route added: https://gist.github.com/saitheexplorer/72c0eef13cf6b9cb357f

I'm stuck if I want to use joi to handle the validation - any ideas? Happy to help with bugfixing if somebody can point me in the right direction.

Thanks!

[mtharrison]

@saitheexplorer thanks for the detailed report.

First worth checking: @hueniverse, is adding default route validation at connection/server level something we actually support?

---

Scenario 1 is because hapi is trying to merge the properties from your plain object from the defaults into a Joi object, which doesn't work. If it is something we should support, we should check that we never try to recursively merge into a property of an object that contains a Joi object.

Scenario 2 is related, the validation for /test route is stomping over the connection defaults because Hoek isn't cloning the Joi object from the defaults.

[nlf]

seems just as easy to wrap a plain object in Joi.object and use Joi.concat to merge them together, no?

[devinivy]

For the sake of brainstorming, another idea is to have the ability to specify whether certain connection defaults should be shallow-copied versus merged, a la Hoek.applyToDefaultsWithShallow().

[mtharrison]

@nlf would concat()'ing them give you what you expect though? If you look at the OP's report it wouldn't seem so.

const Joi = require('joi');

// Connection level

const schema1 = Joi.object({
    api_key: Joi.string().required()
});

// Route level

const schema2 = Joi.object().keys({
    email: Joi.string(),
    user_id: Joi.string()
}).or('email', 'user_id');

const schema = schema1.concat(schema2);

const input = { email: '[email protected]' };

Joi.assert(input, schema);

The output:

ValidationError: {
  "email": "[email protected]",
  "api_key" [1]: -- missing --
}

[1] "api_key" is required

Not sure about others, but I would expect the 2nd to completely override the connection default and not have to specify an api_key

[nlf]

i guess it just raises the question of should behavior be merge or overwrite? i think (i haven't looked) currently it's overwrite for everything else, if that's the case you're right it shouldn't even bother concatting

[mtharrison]

It's currently using Hoek.applyToDefaultsWithShallow(), if we want shallow copy on all route-level validation properties, it should just be a case of adding the relevant keys to be shallow copied:

this.settings = Hoek.applyToDefaultsWithShallow(base, route.config || {}, ['bind', 'validate.headers', 'validate.payload', 'validate.params', 'validate.query']);

and some tests, along the lines of:

it('overrides connection level blah...', (done) => {

    const server = new Hapi.Server();
    server.connection({
        routes: {
            validate: {
                query: Joi.object({
                    a: Joi.string().required()
                }),
                options: {
                    abortEarly: false
                }
            }
        }
    });

    const handler = function (request, reply) {

        return reply('ok');
    };

    server.route({
        method: 'GET',
        path: '/',
        handler
    });

    server.route({
        method: 'GET',
        path: '/other',
        handler,
        config: {
            validate: {
                query: Joi.object({
                    b: Joi.string().required()
                })
            }
        }
    });

    server.inject({ url: '/', method: 'GET' }, (res1) => {

        expect(res1.statusCode).to.equal(400);
        expect(res1.result.message).to.equal('child "a" fails because ["a" is required]');

        server.inject({ url: '/?a=1', method: 'GET' }, (res2) => {

            expect(res2.statusCode).to.equal(200);

            server.inject({ url: '/other', method: 'GET' }, (res3) => {

                expect(res3.statusCode).to.equal(400);
                expect(res3.result.message).to.equal('child "b" fails because ["b" is required]');

                server.inject({ url: '/other?b=1', method: 'GET' }, (res4) => {

                    expect(res4.statusCode).to.equal(200);
                    done();
                });
            });
        });
    });
});

// Without patch to route.js, the first assertion fails with 'Expected 'child "b" fails because ["b" is required]' to equal specified value' because it inherits /other routes validation
// Expected 'child "b" fails because ["b" is required]' to equal specified value

So as you say, the thing to establish is what we should do. Clearly merging with Joi objects (unless we use concat, which might be weird) is pretty fraught so the above seems to me like a reasonable idea.

This would also solve the scenario 2 issue which is related to Hoek not cloning Joi objects.

[hueniverse]

@mtharrison you really made me copy paste what should have been a PR? :-)

This is a bug and the only valid solution is to override.