Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected interaction between Joi.object().unknown() and stripUnknown #983

Closed
cdhowie opened this issue Sep 15, 2016 · 9 comments
Closed
Assignees
Labels
bug Bug or defect
Milestone

Comments

@cdhowie
Copy link

cdhowie commented Sep 15, 2016

Context

  • node version: 4.5.0
  • joi version: 9.0.4
  • environment (node, browser): node

What are you trying to achieve or the steps to reproduce ?

There's a bit of ambiguity between the object().unknown() schema and the { stripUnknown: { objects: true } } validation options. I would expect the stripUnknown option to leave unknown keys alone if the schema declares that it allows unknown keys, but this is not the case; it removes unknown keys regardless of whether the schema permits them.

This puts me in a rather difficult position of having to know which schemas permit unknown keys and which do not, so that I can supply the correct stripUnknown option during validation. My requirements:

  • If an object schema does not allow unknown keys, they are removed without causing an error.
  • If an object schema allows unknown keys, they are ignored.

This particular use case seems to be missing from Joi. Perhaps we could add { stripUnknown: { objects: 'unallowed' } } to cover this? I'm more than happy to submit a PR to add this but want to make sure that this approach would be accepted before I start working on it.

'use strict';

const Joi = require('joi');

let schema = Joi.object().keys({
    a: Joi.string().required()
});

let obj = {
    a: "foo",
    b: "bar"
};

console.log(schema.validate(obj));

console.log(schema.validate(obj, { stripUnknown: { objects: true } }));

console.log(schema.unknown().validate(obj));

console.log(schema.unknown().validate(obj, { stripUnknown: { objects: true } }));

Which result you had ?

{ error:
   { [ValidationError: "b" is not allowed]
     isJoi: true,
     name: 'ValidationError',
     details: [ [Object] ],
     _object: { a: 'foo', b: 'bar' },
     annotate: [Function] },
  value: { a: 'foo', b: 'bar' } }
{ error: null, value: { a: 'foo' } }
{ error: null, value: { a: 'foo', b: 'bar' } }
{ error: null, value: { a: 'foo' } }

What did you expect ?

{ error:
   { [ValidationError: "b" is not allowed]
     isJoi: true,
     name: 'ValidationError',
     details: [ [Object] ],
     _object: { a: 'foo', b: 'bar' },
     annotate: [Function] },
  value: { a: 'foo', b: 'bar' } }
{ error: null, value: { a: 'foo' } }
{ error: null, value: { a: 'foo', b: 'bar' } }
{ error: null, value: { a: 'foo', b: 'bar' } }
@Marsup
Copy link
Collaborator

Marsup commented Sep 15, 2016

I'd say those rules are in conflict, and in many cases joi doesn't try to solve this as it's unnecessary added complexity. How would you even decide which rule wins ?

@cdhowie
Copy link
Author

cdhowie commented Sep 15, 2016

That's why I propose an extra value that informs Joi which one should win. The problem comes into play when you don't know exactly which schema you're dealing with, but would prefer that it remove unknown keys over throwing an error only if those keys would cause an error in the first place. For example, when using Joi as the basis for validating objects that go into a document store, you don't have specific knowledge about the schema you can apply to decide whether to strip unknown keys.

The problem becomes much more difficult when dealing with nested object types where some allow unknown keys and some do not -- in that case, there isn't even a single value of unknownKeys.objects I can use to get the behavior I desire. I'm simply SOL, with no clear path to achieve this behavior.

@Marsup Marsup self-assigned this Mar 18, 2017
@Marsup Marsup added breaking changes Change that can breaking existing code bug Bug or defect and removed breaking changes Change that can breaking existing code labels Mar 18, 2017
@Marsup Marsup closed this as completed in 374f52e Mar 18, 2017
@Marsup Marsup added this to the 10.3.0 milestone Mar 18, 2017
@Marsup
Copy link
Collaborator

Marsup commented Mar 18, 2017

As you can see from above, I hesitated between breaking or non breaking. After thinking about it, I considered that, like in a few other already existing cases in joi, the local choice should win over the global options. I will thus consider this a bug and support it without any extra configuration value.

Anyone considering this is a mistake can come to me fast before I release, but I believe this is the least surprising behavior joi should adopt.

(And sorry for the delay)

@cdhowie
Copy link
Author

cdhowie commented Mar 22, 2017

@Marsup Thanks for taking the time to address this.

Note that I was able to find a workaround for users who may be unable to upgrade. Using .pattern(/.?/, Joi.any()) will have the same effect as .unknown() but will not cause attributes to be stripped with { stripUnknown: { objects: true } }.

@AdrienHorgnies
Copy link

I'm having a similar issue. I'm using both option in the same direction, meaning I don't want extraneous keys.
But using the unknown(false), I'm expecting to get errors if there is any unknown key and I expect them to be stripped as I used the option stripUnknown: true. But it seems the latter act before the former and it is thus being ignored.

So contrary to what was previously said, global settings won over local settings

Marsup : [...] the local choice should win over the global options. [...]

Here is an example of the above mentioned conflict :

const joi = require('joi');
const schema = {
	user: joi.string()
}

const joiObject = joi.object(schema).unknown(false);

const input = {
	user: 'root',
	hello: 'hello',
}

const {error, value} = joiObject.validate(input, { stripUnknown: true });

console.log('ERROR');
console.log(JSON.stringify(error, null, 2));
console.log('INPUT');
console.log(JSON.stringify(input, null, 2));
console.log('OUTPUT');
console.log(JSON.stringify(value, null, 2));

output :

ERROR
null
INPUT
{
  "user": "root",
  "hello": "hello"
}
OUTPUT
{
  "user": "root"
}

Error shouldn't be null.

@cdhowie
Copy link
Author

cdhowie commented Oct 13, 2017

@AdrienHorgnies The trick here is that .unknown(false) is redundant -- that's the default. It's a binary option. You either permit other keys or you do not.

The stripUnknown setting uses the configured value of .unknown() to inform whether or not it should remove keys that aren't in the schema. There's four possible situations:

  • .unknown(false) and stripUnknown: false -- Extra keys cause an error.
  • .unknown(false) and stripUnknown: true -- Extra keys are removed without an error.
  • .unknown(true) and stripUnknown: false -- Extra keys are ignored without an error.
  • .unknown(true) and stripUnknown: true -- Extra keys are ignored without an error.

To put it another way, .unknown() configures the schema object in regards to whether unknown keys are permitted. The global stripUnknown option configures the validation process in regards to what should we do if forbidden unknown keys are encountered.

I'm having trouble making sense of this statement:

I'm expecting to get errors if there is any unknown key and I expect them to be stripped

I can't imagine a situation when you would want both -- for the keys to be stripped and to get an error. You usually want one or the other, and stripUnknown is how you choose which.

@AdrienHorgnies
Copy link

AdrienHorgnies commented Oct 16, 2017

@cdhowie The situation is that I am validating a configuration file. I wanted to be able to validate the file, warn the user about any extraneous keys (warnings based on validation errors) but still go on with what was valid (stripped input, I cannot ignore extraneous keys as I'm using Object.keys(configuration)). I would then prompt any missing information and update the configuration file myself.

I thought that would be a nice feature and that Joi was the right one to do the job. It's already almost there.

@cdhowie
Copy link
Author

cdhowie commented Oct 16, 2017

@AdrienHorgnies A workaround would be validating twice.

To warn the user, validate with { abortEarly: false }, which will cause Joi not to stop on the first error, collecting all of the issues which you can then present to the user.

To obtain just the valid data with bad keys removed, validate with { stripUnknown: true }.

@AdrienHorgnies
Copy link

There are indeed plenty of solutions I could implement. I presented a use case that highlighted the feature that it could be.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug or defect
Projects
None yet
Development

No branches or pull requests

3 participants