ExodusMovement / schemasafe

A reasonably safe JSON Schema validator with draft-04/06/07/2019-09/2020-12 support.
https://npmjs.com/@exodus/schemasafe
MIT License
161 stars 12 forks source link

Use obj.x instead of obj["x"] in common safe cases #96

Closed ChALkeR closed 4 years ago

ChALkeR commented 4 years ago

Actually, two loosely related changes here.

1) obj.x -> obj["x"] for properties with names /^[a-z][a-z0-9_]*$/i.

Note how this check is much less complex than what is-my-json-valid had (which attempted to inline _all_ valid property literals, so the regex was quite big). Approach in this PR should be trivial and safe.

Motivation: to make the generated code a bit more readable.
I expect that most properties will fit into that regexp.

2) As safe() ids now extended to case-insensitive, we can use it to wrap scope keys in generate-function, which double-checks that scope keys are safe for inclusion.

ChALkeR commented 4 years ago

E.g.

{
  properties: {
    x: { type: 'string' },
    '0x': { type: 'string' },
  }
}

now generates:

(function() {
'use strict'
const hasOwn = Function.prototype.call.bind(Object.prototype.hasOwnProperty);
return (function validate(data) {
  if (data === undefined) data = null
  let errors = 0
  if (typeof data === "object" && data && !Array.isArray(data)) {
    if (data.x !== undefined && hasOwn(data, "x")) {
      if (!(typeof data.x === "string")) return false
    }
    if (data["0x"] !== undefined && hasOwn(data, "0x")) {
      if (!(typeof data["0x"] === "string")) return false
    }
  }
  return errors === 0
})})();
kklash commented 4 years ago
  1. What is the motivation for this change? Does it affect performance? We're moving back to the way is-my-json-valid handled this, which IMO was inconsistent and not necessary
  2. Does this affect the paths shown in error messages? Shouldn't we keep the format consistent so that callers can parse the path in a machine-readable way if they need to? If the caller has to account for both .foo and ['foo'] property notation, this would complicate matters for them
ChALkeR commented 4 years ago

Error messages are not affected as they use JSON pointers and got decoupled from this logic. JSON pointers are easily machine-readable.

Motivation is to make the generated code a bit more readable (and shorter) -- this way, most properties in schemas will use shorter naming, i.e. will result in e.g. data.x.y.z.u instead of data["x"]["y"]["z"]["u"] in deep-nested property paths.

I don't think that inconsistent naming is a significant issue, as most property names should fall into /^[a-z][a-z0-9_]*$/i. Or even all properties in most schemas.

Re: speed -- I will be very surprised if this affects runtime speed in any way. It shouldn't.

imjv used this regexp: https://github.com/mikolalysenko/is-property/blob/master/is-property.js#L3, which is too large and could (and did) contain errors, also at that time error paths were not decoupled from this logic.

This PR whitelists a much smaller and more compact set of property names, so I don't expect any problems.