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

removeAdditional doesn't work well with anyOf, oneOf or allOf #129

Closed
gtrindade opened this issue Feb 22, 2016 · 7 comments

Comments

@gtrindade
Copy link

commented Feb 22, 2016

Hi, first of all, awesome work with this validator! Very handy!

Now here is the problem I'm facing, I'm not sure if this is the expected behavior, but if it is, it's not very intuitive... my scenario is a bit more complex, but I'll simplify in the following example.

I'm initializing ajv with:

const ajv = Ajv({removeAdditional: true})

This is the sample schema:

const schema = {
    type: `object`,
    additionalProperties: {
      anyOf: [{
        type: `object`,
        properties: {
          a: {
            type: `string`
          }
        },
        required: [`a`],
        additionalProperties: false
      }, {
        type: `object`,
        properties: {
          b: {
            type: `string`
          }
        },
        required: [`b`],
        additionalProperties: false
      }]
    }
  }

It should accept an object with as many objects as we want, and each of these objects should have at least one property called a, or one called b. If a or b are not present, it should fail. It should also discard any other property that is not a or b.

Now, with this data:

  const data1 = {
    obj1: {
      a: `a`
    },
    obj2: {
      b: `b`
    },
    obj3: {
      c: `c`
    }
  }

It should fail only on obj3, but that's only the case when removeAdditional is false. If removeAdditional is true, it seems like as soon as it evaluates the first option of the anyOf, it removes the additional properties of the object before trying the second option.

the removeAdditional is very useful, but it needs to wait until all possibilities of oneOf to be evaluated before removing anything.

This is the error I get by running the code above:

     Error: the array [
  {
    "dataPath": "['obj2']"
    "keyword": "required"
    "message": "should have required property 'a'"
    "params": {
      "missingProperty": "a"
    }
    "schemaPath": "#/additionalProperties/anyOf/0/required"
  }
  {
    "dataPath": "['obj2']"
    "keyword": "required"
    "message": "should have required property 'b'"
    "params": {
      "missingProperty": "b"
    }
    "schemaPath": "#/additionalProperties/anyOf/1/required"
  }
  {
    "dataPath": "['obj2']"
    "keyword": "anyOf"
    "message": "should match some schema in anyOf"
    "params": {}
    "schemaPath": "#/additionalProperties/anyOf"
  }
  {
    "dataPath": "['obj3']"
    "keyword": "required"
    "message": "should have required property 'a'"
    "params": {
      "missingProperty": "a"
    }
    "schemaPath": "#/additionalProperties/anyOf/0/required"
  }
  {
    "dataPath": "['obj3']"
    "keyword": "required"
    "message": "should have required property 'b'"
    "params": {
      "missingProperty": "b"
    }
    "schemaPath": "#/additionalProperties/anyOf/1/required"
  }
  {
    "dataPath": "['obj3']"
    "keyword": "anyOf"
    "message": "should match some schema in anyOf"
    "params": {}
    "schemaPath": "#/additionalProperties/anyOf"
  }
] was thrown, throw an Error :)

As you can see, obj2 is failing because it's missing property b, which is not true.

I called validate with

const result = ajv.validate(schema, data1)
@epoberezkin

This comment has been minimized.

Copy link
Owner

commented Feb 23, 2016

It works as it should in this situation. Look at this working example.

The first instance of ajv does not have removeAdditional option. And it correctly fails on obj3.

The second instance has removeAdditional option as true. When it validates obj1 it passes because the first schema in anyOf has 'a' property in properties. When it validates obj2 using the first schema in anyOf property 'b' is additional there. So it gets removed, as you can see from the log. But the first schema in anyOf fails and it goes on to try to validate it with the second schema. It fails too because there is no property 'b' (it was removed).

{ removeAdditional: true } removes any property that would otherwise be validated with the schema in additionalProperties: false keyword in the same schema. It does not try to establish whether this property is known to some other branch of the schema - it is simply out of its scope. There is a place for defining unknown properties in the standard - there is actually this proposal for v5. But it is not what this option is doing.

I don't think there is a real need to define the concept of "unknown property" because usually there are ways to refactor your schema to make it work (and remove what you need). Difficult to say how exactly of course without seeing the actual schema, but usually you just need to take out properties from keywords anyOf, oneOf. The example above doesn't need to have properties inside anyOf. The equivalent (and faster) refactored schema is:

{
  type: 'object',
  additionalProperties: {
    type: 'object',
    properties: {
      a: { type: 'string' },
      b: { type: 'string' } 
    },
    additionalProperties: false,
    anyOf: [
      { required: ['a'] },
      { required: ['b'] }
    ]
  }
}

With this schema it will correctly fail on obj3 if you pass { removeAdditional: true }. It can also filter out obj3 if you pass { removeAdditional: 'failing' } (see docs).

@gtrindade

This comment has been minimized.

Copy link
Author

commented Feb 23, 2016

Thanks for the suggestion, I didn't realize I could combine anyOf with required. I'll give it a try!

I'm not sure if this is just me, but took me quite a while to understand this behavior of the removeAdditional option... To me, it feels like it should only remove whats not matching anything. In this case, it should wait until all the options on anyOf to be evaluated before removing anything.

But again, thanks for your quick response!

@epoberezkin

This comment has been minimized.

Copy link
Owner

commented Feb 23, 2016

It just that it uses the same definition of "additional" as JSON-schema standard - it means "additional" according to the current (sub)schema, i.e. not present in "properties" and not matching "patternProperties" of the same (sub)schema, and it does not mean "unknown" according to the whole schema. For example, this schema should fail on any data because of that:

{
  type: 'object',
  properties: { foo: { type: 'string' } },
  required: ['foo'],
  allOf: [ { additionalProperties: false } ]
}

This would alway fail too:

{
  type: 'object',
  required: ['foo'],
  additionalProperties: false
}

But it definitely not just you, it takes some time to understand how the standard works... The main thing here is that all the keywords are shallow, they can only be affected by siblings in the same object.

I will think again about implementing this "unknown" properties concept - both to fail on unknown properties and to filter them out. There are cases when it is either difficult to refactor schema (I haven't come across the cases where it is impossible though...) or undesirable, because it makes schema lose it's meaning from the application domain point of view.

@epoberezkin

This comment has been minimized.

Copy link
Owner

commented Feb 25, 2016

Closing this issue, please let mw know if there are any questions.

@epoberezkin

This comment has been minimized.

Copy link
Owner

commented Oct 14, 2016

The easiest is to use v5 switch keyword instead of oneOf (and by the way, anyOf is often faster than oneOf - for 50% of data in your case). Otherwise see suggestions in issues mentioned in FAQ

@sberan

This comment has been minimized.

Copy link

commented Aug 26, 2017

@epoberezkin would you accept a PR that removed the additional properties only after passing validation of the schema? That way, if a subschema fails validation for some other reason, the properties aren't removed.

Something like removeAdditional: 'afterValidation'? I think this would be more intuitive in these cases.

@epoberezkin

This comment has been minimized.

Copy link
Owner

commented Aug 26, 2017

@sberan so rather than removing them straight away, additionalProperties: false would ignore them and once the sub-schema validation passed, you would remove? Maybe it's possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.