Don't rely on splitting on periods; explicitly disallow arbitrary keys in transitions

[lbud]

Alternative to #558.

While it's doubtful anyone is making up arbitrary keys in transitions in a style, I'd still rather not make the spec any less strict — object validation now relies on a boolean option to error on arbitrary keys (so any other properties that go through vanilla object validation can have arbitrary keys).

@jfirebaugh @lucaswoj

[lbud]

@lucaswoj @jfirebaugh

To make sure I fully understood this, I broke down the validation pipeline here (and caught a few bugs with the light validation I had written here :pray:):

Here's what happens when we validate a style:

• A full style enters the validation pipeline here. It ($root) is a non-function object, and it doesn't have a spec-defined type or a dedicated validator, so it enters the object validator
• The keys at the root level either go into their own object element validators, or they're arbitrary keys and at the root level they are tolerated.
  • version: enum validator
  • name: string validator
  • metadata: wildcard passthrough validator
  • center: array validator -> number validator
  • zoom: number validator
  • bearing: number validator
  • pitch: number validator
  • light: light validator
  • *-transition: validated as transition types, disallowArbitrary=true
  • other keys: validated as one of (array, number, enum, color)
  • sources: source validator. Each key is validated through the * line here/here as a source type
  • vector and raster sources with a url specified explicitly disallow arbitrary keys here
  • none of the other types (or vector or raster sources without the url key) seem to explicitly disallow arbitrary keys — if this is the case it also seems like something we would break if we started enforcing disallowArbitrary only now
  • sprite: string validator
  • glyphs: glyph validator
  • transition ($root-level): would validate as a vanilla object and thus allow arbitrary keys. Should we add something to disallow these?
  • layers: array validator -> layer validator
  • ultimately these get to this megavalidator:
    • filter: filter validator (does not re-enter object validation)
    • layout: re-enters the object validator with every key/value being subjected to the layout prop validator, effectively disallowing arbitrary sub-layout keys
    • individual layout properties: layout prop validator; no layout properties are non-validator-primitive types (i.e. re-enter object validator)
    • via the previous .split('.') mechanism arbitrary keys were disallowed
    • paint: re-enters the object validator with every key/value being subjected to the paint prop validator, effectively disallowing arbitrary sub-paint keys
    • individual paint properties: paint prop validator; no vanilla paint properties are non-validator-primitive types (i.e. re-enter the object validator) except -transition properties
    • via the previous .split('.') mechanism arbitrary keys were disallowed
    • function values enter the function validator; by the previous .split('.') mechanism arbitrary keys were disallowed
    • id: string validator
    • type: enum validator
    • metadata: wildcard passthrough validator
    • ref: string validator (+ does additional ref validation here)
    • source: string validator
    • source-layer: string validator
    • minzoom: number validator
    • maxzoom: number validator
    • any other keys are permitted as arbitrary keys

---

• Should allowArbitrary be true by default? I would like to make new style spec primitives as strict as possible. Strictness should be the default.

To clarify, it's disallowArbitrary in this PR but your point stands. We could switch this to allowArbitrary (false by default) and allow it only in the root object and root layers.

• Are you sure that transition objects are the only ones that have been enforcing "strictness"? What about light objects? What about layout / paint objects within layers? What about function objects?

See above. (light object has its own validator, which I just fixed some bugs in and added tests to make sure we don't tolerate arbitrary keys. (Note that the root-level transition hasn't been prohibiting arbitrary keys — should it?) As shown in https://github.com/mapbox/mapbox-gl-style-spec/pull/562, layout and paint objects as well as function keys have been enforcing strictness via the splitting-on-periods mechanism. (I merged that PR then rebased here to ensure this enforces strictness in the same way.)

• We already have a means of interacting with arbitrary keys in objects: the * wildcard object element validator. What would it mean to specify a * wildcard object element validator and make allowArbitrary false? Could the two concepts be combined? For example, we could allow arbitrary keys iff a * wildcard object element validator is specified.

The wildcard object element validator is more for when "type": "*", rather than for interacting with wildcard keys themselves. I'm not sure we should conflate these?

• Is allowArbitrary a specific enough name for this setting that someone who stumbles across it in the codebase without context would know what it means? "Allow arbitrary what?"

I'm happy to change to disallowArbitraryKeys (or, if we go with the change mentioned above, allowArbitraryKeys).

---

In my opinion then the outstanding questions are

• should we add strictness (disallowArbitrary) to the $root-level global transition?
• should we rename this from disallowArbitrary to allowArbitrary? (`.concat('keys')) see below — done
• should we toggle this to strict by default, and instead be explicit in all the places where we do allow arbitrary keys, so that future object validation will be strict by default? see below — done

[lbud]

• should we rename this from disallowArbitrary to allowArbitrary? (`.concat('keys'))
• should we toggle this to strict by default, and instead be explicit in all the places where we do allow arbitrary keys, so that future object validation will be strict by default?

Just did in https://github.com/mapbox/mapbox-gl-style-spec/pull/559/commits/428bf80f1941ed3ebe76367db64f849c8a0f4075 https://github.com/mapbox/mapbox-gl-style-spec/pull/559/commits/f902b4a9a45aa679f068544f6834749e4db6d4f7

[lucaswoj]

Thank you for being so thorough on this 🙇

---

should we add strictness (disallowArbitrary) to the $root-level global transition?

A stated goal of this PR is to not increase strictness. We should not add strictness to the $root-level global transition.

The wildcard object element validator is more for when "type": "*", rather than for interacting with wildcard keys themselves. I'm not sure we should conflate these?

What would it mean to specify a * wildcard object element validator and when allowArbitrary false? Or is that combination impossible for a reason I'm missing? (It's been a while since I wrote this code.)

It is desirable that it be impossible to pass contradictary arguments to this method. It is desirable that objects which allow arbitrary keys must go against the grain to omit validation for arbitrary keys.