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

Add "schemaPath" to verbose output, showing which subschema triggered each error. #148

Merged
merged 2 commits into from
Dec 18, 2017

Conversation

deBhal
Copy link
Contributor

@deBhal deBhal commented Dec 1, 2017

This PR adds a 'schemaPath' to the verbose output that can be used to get back to the failed rule in the schema in order to associate metadata with failures.

This provides a solution to #102 and #122 by making it possible to pull the appropriate additionalProperties or enum out of the schema.

For example, given this schema:

{	"id": "birds",
	"allOf": [
		{ 	"metadata": "ravens_are_black",
		 	"not": {
				"properties": {
					"animal": { "enum": [ "raven" ] },
					"color": {
						"not": {
							"enum": [ "black" ] } } } } },
		{	"metadata": "nested_string",
			"properties": {
				"outer": {
					"properties": {
						"inner": {
							"metatdata": "deep",
							"type": "string" } } } } },
		{	"metadata": "doves_are_white",
			"not": {
				"properties": {
					"animal": { "enum": [ "dove" ] },
					"color": {
						"not": {
							"enum": [ "white" ] } } } } } ] }

You can validate trace back failures to the
and this test program:

const validator = require( './index.js' );
const birdsSchema = require('./bird-schema.json');

const badRaven = { animal: 'raven', color: 'rainbow', outer: { inner: 12 } };
const badDove = { animal: 'dove', color: 'pink' };

const validate = validator( birdsSchema, { greedy: true, verbose: true } );
const get = require( 'lodash/get' );

function printError( error ) {
	console.log( 'error:', JSON.stringify(error) );
	const failedRule = get( birdsSchema, error.schemaPath );
	console.log( 'failedRule:', JSON.stringify( failedRule ) );
	console.log();
}

validate( badRaven );
console.log('badRaven:');
validate.errors.map( printError );

console.log();

validate( badDove );
console.log("badDove:");
validate.errors.map( printError );

We can get the correct rule and it's metadata back out of the schema:

badRaven:
error: {"field":"data","message":"negative schema matches","value":{"animal":"raven","color":"rainbow","outer":{"inner":12}},"schemaPath":["allOf",0]}
failedRule: {"metadata":"ravens_are_black","not":{"properties":{"animal":{"enum":["raven"]},"color":{"not":{"enum":["black"]}}}}}

error: {"field":"data.outer.inner","message":"is the wrong type","value":12,"type":"string","schemaPath":["allOf",1,"properties","outer","properties","inner"]}
failedRule: {"metatdata":"deep","type":"string"}


badDove:
error: {"field":"data","message":"negative schema matches","value":{"animal":"dove","color":"pink"},"schemaPath":["allOf",2]}
failedRule: {"metadata":"doves_are_white","not":{"properties":{"animal":{"enum":["dove"]},"color":{"not":{"enum":["white"]}}}}}

The patch adds only the extra static string to the generated validation functions, so it should have virtually no effect on performance. (There's a very modest amount of extra work being done at compile time).

I added paths for anyOf and oneOf, but then I pulled it out again. I think it doesn't make sense to dive into them.

I've added paths to every call to visit, but I haven't exercised any of it, so they're probably wrong right now and need some tests.

@deBhal deBhal changed the title track schemaPath and print if verbose [WIP] Track schemaPath and print if verbose Dec 1, 2017
@deBhal deBhal changed the title [WIP] Track schemaPath and print if verbose Add "schemaPath" to verbose output to show which subschema triggered each error. Dec 2, 2017
@deBhal deBhal changed the title Add "schemaPath" to verbose output to show which subschema triggered each error. Add "schemaPath" to verbose output, showing which subschema triggered each error. Dec 2, 2017
Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@LinusU
Copy link
Collaborator

LinusU commented Dec 4, 2017

I'll give @mafintosh a few days to look, otherwise I'll merge it 👍

Feel free to ping me if (when) I forget 😄

@deBhal
Copy link
Contributor Author

deBhal commented Dec 5, 2017

@LinusU: Thanks for looking at it so quickly! I wasn't quite finished :p

I've added some tests, fixed a bug they turned up, updated the docs and squashed, so I think I'm done now :)

I've just realized that squashing is going to make it harder for you to see the changes :( The only semantic difference is the bugfix, where I added added a check for tuple before pushing properties onto the path here:
https://github.com/mafintosh/is-my-json-valid/pull/148/files#diff-168726dbe96b3ce427e7fedce31bb0bcR548
(this is because we re-use properties for arrays here, and so I was adding an incorrect properties field to the path before every array index).

I did also have a look at #38 and #22 which both talked about something like this, but I don't think it fits here.

When oneOf fails, whether it's because 2 rules passed or none you can't point to any of those sub rules and it was the wrong one. The failure is in the oneOf. Same with anyOf.

I'm a fan of the the feature those issues describe, and I'd be keen to see it implemented in some form, I'm just saying that I don't think it fits into the path.

README.md Outdated
verbose: true
})

validate({hello: 100});
console.log(validate.errors) // {field: 'data.hello', message: 'is the wrong type', value: 100, type: 'string'}
console.log(validate.errors, null, 2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that null, 2 was intended for a JSON.stringfy call and not a console.log one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks :) Fixed.

Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nit 👍

@LinusU
Copy link
Collaborator

LinusU commented Dec 5, 2017

I've just realized that squashing is going to make it harder for you to see the changes :(

No problem at all 👌

I'm a fan of the the feature those issues describe, and I'd be keen to see it implemented in some form, I'm just saying that I don't think it fits into the path.

👍

@deBhal
Copy link
Contributor Author

deBhal commented Dec 17, 2017

Feel free to ping me if (when) I forget 😄

I know that feeling so well. Ping :)

@LinusU LinusU merged commit 928417d into mafintosh:master Dec 18, 2017
@LinusU
Copy link
Collaborator

LinusU commented Dec 18, 2017

Published as 2.17.0 🚀

Thank you for your contribution!

@groupsky
Copy link

node 5.12.0, same error: TypeError: Cannot read property 'concat' of undefined

@LinusU
Copy link
Collaborator

LinusU commented Dec 18, 2017

Shit, working on this!

@LinusU
Copy link
Collaborator

LinusU commented Dec 18, 2017

If anyone could give me a stacktrace I would appreciate it!

@jonanone
Copy link

@LinusU Check the open PRs regarding the issue #150 #151 #152 It will give you some hints for sure.

@LinusU
Copy link
Collaborator

LinusU commented Dec 18, 2017

Yeah, just say 😄

Fix published as 2.17.1, so sorry for any inconvenience caused!

@deBhal
Copy link
Contributor Author

deBhal commented Dec 18, 2017

so sorry for any inconvenience caused!

Me too, Sorry! :'(

I feel pretty stupid about it having seen the fix - I added a bunch of tests but not the right ones :(

Breaking downstream packages is a first for me, and I'd love to never do it again - is there something I could/should have done to catch this ahead of time?

@LinusU
Copy link
Collaborator

LinusU commented Dec 19, 2017

I feel pretty stupid about it having seen the fix

Don't beat yourself up, I missed it too!

is there something I could/should have done to catch this ahead of time?

Not really, we should definitely look at getting code coverage up and running though, and I would love to migrate to TypeScript which would have caught this error as well. Other than that it's just trying to add as many tests as possible 😄

@deBhal deBhal deleted the add/schemaPath branch December 26, 2017 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants