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

Errors for 'required' validation should include missing property #18

Closed
cdm6a opened this issue Jun 29, 2015 · 9 comments
Closed

Errors for 'required' validation should include missing property #18

cdm6a opened this issue Jun 29, 2015 · 9 comments

Comments

@cdm6a
Copy link

cdm6a commented Jun 29, 2015

Hi, I'm trying to get some clarity on a potential issue I'm seeing. In running the following:

const ajv = Ajv({verbose: false, allErrors: true});

ajv.addSchema([
   require('./schemas/Currency.json'),
   require('./schemas/PositiveFloatString.json'),
   require('./schemas/Amount.json'),
   require('./schemas/Party.json'),
])

function isValid(object, schemaName) {
  return ajv.validate(schemaName, object);
}

function validate(object, schemaName) {
  if (!isValid(object, schemaName)) {
    return {
      isValid: false,
      errors: ajv.errors
    };
  }
  return {
    isValid: true
  };
}

console.log(validator.validate({
        entity: 'acct:conner@ropple.com',
        amount: {
          currency: 'USD'
        }
      }, 'Party'));

I get the following:

{ 
  isValid: false,
  errors: [{ 
    keyword: 'required',
     dataPath: '.amount',
     message: 'properties value, currency are required' 
  }]
}

Ideally I would like to see something more along the lines of:

{ 
  isValid: false,
  errors: [{ 
    keyword: 'required',
    dataPath: '.amount.value',
    message: 'property value is required' 
  }]
}

I was wondering if this was an issue with my implementation / schemas or if it is a shortcoming of ajv?

Thanks in advance.

My schemas are as follows:
Party.json

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "id": "Party",
  "title": "Party",
  "description": "Represents a single party to a payment.",
  "type": "object",
  "properties": {
    "name": {
      "$ref": "Account"
    },
    "amount": {
      "description": "How much this party will send or receive in the payment.",
      "$ref": "Amount"
    }
  },
  "additionalProperties": false,
  "required": ["name", "amount"]
}

Amount.json

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "id": "Amount",
  "title": "Amount",
  "type": "object",
  "properties": {
    "value": {
      "$ref": "PositiveFloatString"
    },
    "currency": {
      "type": "string",
      "$ref": "Currency"
    }
  },
  "additionalProperties": false,
  "required": ["value", "currency"]
}

PositiveFloatString.json

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "id": "PositiveFloatString",
  "title": "PositiveFloatString",
  "description": "A string representation of a floating point number",
  "type": "string",
  "pattern": "^[+]?[0-9]*[.]?[0-9]+([eE][-+]?[0-9]+)?$"
}

Currency.json

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "id": "Currency",
  "title": "Currency",
  "type": "string",
  "pattern": "^(([a-zA-Z0-9]{3})|([a-fA-F0-9]{40}))$"
}
@epoberezkin
Copy link
Member

On it.

@epoberezkin
Copy link
Member

It is a design choice for performance rather than shortcoming :) Required keyword is validated in a single expression (conditions for each property are connected with &&), not in the loop, so ajv has no knowledge of which specific property is missing.

Having said that I agree that one wants to know what exactly is missing, not just that something is missing :)

I'll see how I can improve it without damaging performance. Maybe it should be an option. Or I have another idea...

Thank you for pointing it out.

@stefk
Copy link

stefk commented Jun 30, 2015

Hi,

I understand performance is an important consideration, but precise error reporting is also really valuable.

I'm currently building a format specification using JSONSchema, and as the multiple schemas involved are subject to many changes at this point, I've set up a test suite automating validation of valid/invalid data samples and generating a primitive documentation above it.

The current error reporting of ajv forces me to make assertions on error messages, which aren't always precise, and the whole approach of relying on message strings is quite fragile. I'd really like to have something like error codes/constants along with better data paths and messages.

@cdmcnamara's proposal:

{ 
  keyword: 'required',
  dataPath: '.amount.value',
  message: 'property value is required' 
}

would allow cleaner assertions on errors like:

keyword === 'required' 
dataPath === '.amount.value

@epoberezkin
Copy link
Member

I agree. I have a solution in mind - it will have missing properties in dataPath without affecting performance. If you use allErrors=true then each missing property will create an error (not one error as it is now). If allErrors is not passed it will return the error with the name of the first missing property. Just need a little time for that.

And thanks for using it :)

@epoberezkin epoberezkin changed the title Nested referenced schema errors not comprehensive Errors for 'required' validation should include missing property Jun 30, 2015
@cdm6a
Copy link
Author

cdm6a commented Jun 30, 2015

Thanks for the quick reply! This solution sounds ideal.

@epoberezkin
Copy link
Member

done in 0.6.1

@stefk
Copy link

stefk commented Jul 1, 2015

Thanks :)

@epoberezkin
Copy link
Member

you're welcome. Thanks for the motivation to improve it :)

@epoberezkin
Copy link
Member

By the way, the error message for "required" was simply listing all required properties, not just missing ones. So making assertions on it wasn't the right thing to do.

This was referenced Oct 29, 2015
epoberezkin added a commit that referenced this issue Nov 19, 2015
… points to the object that is validated and not to the missing property; old (<=1.4.10 ) error reporting of dataPath for "required" keyword is available with option errorDataPath == "property"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants