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

Regression on ref to empty schema crashes JSCK #97

Open
cxreg opened this issue Oct 31, 2015 · 8 comments
Open

Regression on ref to empty schema crashes JSCK #97

cxreg opened this issue Oct 31, 2015 · 8 comments

Comments

@cxreg
Copy link
Contributor

cxreg commented Oct 31, 2015

Starting in 0.2.8 this throws a fatal error. It only appears to happen if you have a reference to an empty schema. An empty schema by itself is fine as are refs in other situations.

var jsck = require("jsck").draft4;
var validator = new jsck({
    "type": "object",
    "properties": { "foo": { "$ref": "#/definitions/foo" } },
    "definitions": { "foo": {} }
});
var res = validator.validate({ "foo": "bar" });

I get this error

count@bumba:~/jsck-test$ ./crash-0.3.0.js

/home/count/jsck-test/node_modules/jsck/lib/validator.js:316
return (_ref1 = _this.uris[uri])._test.apply(_ref1, args);
^
TypeError: Cannot call method 'apply' of undefined
at /home/count/jsck-test/node_modules/jsck/lib/validator.js:316:54
at /home/count/jsck-test/node_modules/jsck/lib/draft4/objects.js:52:15
at Object.test_function as _test
at Object.validate (/home/count/jsck-test/node_modules/jsck/lib/validator.js:109:22)
at Validator.validate (/home/count/jsck-test/node_modules/jsck/lib/validator.js:94:34)
at Object. (/home/count/jsck-test/crash-0.3.0.js:11:21)
at Module._compile (module.js:456:26)
at Object.Module._extensions..js (module.js:474:10)
at Module.load (module.js:356:32)
at Function.Module._load (module.js:312:12)

@automatthew
Copy link
Contributor

Thanks. Will investigate as soon as I can.

@cxreg
Copy link
Contributor Author

cxreg commented Nov 5, 2015

I've dived into this a little bit. I'm not 100% sure why the scoping changes around uri exposed this, but what is happening is that the empty schema fails is_schema because none of the properties are present. That causes it never to call compile() on this schema, and thus the _test method is never assigned.

It seems like is_schema should not be duck typing, since contextually we should know whether or not a schema is expected in a given spot, is that not so?

If it can't be fixed that way, it seems like maybe just conditionally not executing _test if it's not defined is a safe fix

@automatthew
Copy link
Contributor

It seems like is_schema should not be duck typing, since contextually we should know whether or not a schema is expected in a given spot, is that not so?

I think I added is_schema so JSCK could handle structural nesting. E.g.

definitions:
  user:
    create:
      type: 'object'
      required: ['email', 'password']
    update:
      type: 'object'

@automatthew
Copy link
Contributor

If it can't be fixed that way, it seems like maybe just conditionally not executing _test if it's not defined is a safe fix

Maybe. That would produce correct behavior for this case. I think the underlying problem is in the compile_definitions logic. It is plausible that an object in the schema document might be both a valid schema and a container of other schemas. Bad form to do so, in my opinion, but possibly something JSCK should support.

Here's my first thought on how to fix it:

    compile_definitions2: (context, object) ->
      if @is_schema(object)
        @compile(context, object)
      if @test_type "object", object
        for name, definition of object when not_schema_keyword(name)
          @compile_definitions context.child(name), definition

@automatthew
Copy link
Contributor

not_schema_keyword

Clever.

@automatthew
Copy link
Contributor

@cxreg, this branch appears to fix the original problem, while also somewhat rigorizing the approach.

https://github.com/pandastrike/jsck/tree/97-ref-to-empty

@cxreg
Copy link
Contributor Author

cxreg commented Nov 9, 2015

Interesting. I think you still need the @test_type "object" conditionalizing @is_schema so as not to throw TypeError on numbers, strings, null, etc? Would that only happen on a malformed schema? Still probably bad.

This does look like a better approach if you are happy with the results.

@automatthew
Copy link
Contributor

I moved the type test to the top of compile_definitions, to simplify the logic there, leaving is_schema as a function with implied type of object. Perhaps I should inline it.

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

No branches or pull requests

2 participants