Null default with custom validator causes json parse error

[RoboPhred]

When using a command line argument, a custom format function, and a null default, convict attempts to run JSON.parse on the argument.

This is happening in convict 1.0.2, but the code seems to be the same in master.

The issue seems to originate in the getFormat function:

function getFormat(schema, path) {
  var o = traverseSchema(schema, path);
  return o ? (typeof o.format === 'string' ? o.format : typeof o.default) : null;
} 

In the return conditional, o.format is a function and o.default is null, resulting in (typeof null) == "object".
This causes the coerce function to try and JSON.parse the content.

This conflicts with the FAQ, stating: "However, you can set a default value to null and if your format doesn't accept null it will throw an error." (#29)

I am currently working around this by setting default to an empty string, as my use case is to check for certain URL formats and I can validate against it.

Example:

var schema = {
  arg: "amqp-endpoint",
  format: function validate(value) {
    // Custom logic here
    if (value == null) throw new Error("amqp endpoint must be specified");
    return true;
  },

  // default is set to null as per FAQ
  default: null
}

node foo.js -- --amqp-endpoint=amqp://localhost
Error: Unexpected token a

[madarche]

Thanks for reporting @RoboPhred. We will look into it ASAP.

[madarche]

@RoboPhred could you make a PR consisting only of a failing test please? This will be easier for everyone interested in solving this to jump in. Thanks again!

[RoboPhred]

Sent a pull request with only a test case.

I have a possible fix as well:
https://github.com/RoboPhred/node-convict/commit/a14953452d96391ed84f65f46b18cdc5ca371a3c
This passes all of the test cases, but I am not sure of the consequences of returning null from getFormat. Particularly, coerce() has a TODO about throwing an exception, which if implemented will break this use-case again.

[madarche]

@RoboPhred thanks for all the care you put into this. I'll look at it in the next few days. Unfortunately very busy right now. If someone else can review it too it could go faster.

[madarche]

Your PR has been merged. Thank you very much @RoboPhred and sorry for all the delay.