microsoft / typespec

https://typespec.io/
MIT License
3.9k stars 176 forks source link

Allow configuration of `unevaluatedProperties` in JSON Schema emitter, and possibly change the default behaviour of all models. #3549

Open adamscybot opened 1 month ago

adamscybot commented 1 month ago

Clear and concise description of the problem

Currently, there exists no (non-workaround) decorator in order to configure unevaluatedProperties on a schema. If looking to restrict unknown properties, I would hazard unevaluatedProperties: false is usually the tool that you should reach for as opposed to additionalProperties: false -- especially where you have TypeSpec in play which induces dev expectations/assumptions about semantics of things like extends.

That's because additionalProperties: false will restrict properties to those defined on the specific schema, and so prevent the use of properties on related schemas (via allOf for example).

In that sense additionalProperties: false seals the definition and would not actually work with any properties coming from some other model via extends, which is weird semantics.

Incidentally, there is no way to set additionalProperties: false either unless you override it with @extension("additionalProperties", Json<false>). But perhaps that should be added as a sort of @sealed decorator.

We could add a @noUnknown decorator or something that controls unevaluatedProperties . At the moment you have to do @extension("unevaluatedProperties", Json<false>) which feels like a hack.

Perhaps it could be put into one like @strictProperties(preventExtension: boolean) where preventExtension would be false by default. if false, it would add unevaluatedProperties: false to the schema. If true, it would instead use additionalProperties: false, which would ideally a throw an error if used when the schema attempts to use index types (including Record), or if that schema is extended by another.

Note, not being able to define these properties using a first class API is unusual as the default behaviour of JSON Schema is to allow additional properties which is behavior the user has not defined in their schema specifically using Record or index types. Therefore there is probably a debate to be had about if unevaluatedProperties: false should actually be added to schemas by default.

Checklist

adamscybot commented 1 month ago

Some other epiphanies about this.

  1. unevaluatedProperties: false is really just an instruction to not allow properties that are not on some allow list built up dynamically as a result of the "schema evaluation" step in whatever tool you are using. It is sort of "lazy" in nature and is checked after the schemas are "composed" in the wider eval process in real/theoretical tooling.
  2. On the flip side additionalProperties: false is something which changes/restricts the "schema evaluation" step behaviour itself and is a modifier that is being applied during the "composition" step. If a schema A has additionalProperties: false and a schema B does not then:
    • If schema A uses relationships like allOf and includes schema B, then any property present in B and not in A, won't actually allow it, unless they are redeclared.
    • If schema B uses relationships like allOf and includes schema A, then B effectively "adopts" the additionalProperties: false from A (since allof indicates valid against all schemas), effectively meaning any new properties in B that weren't on A, wont work.

In the context of typespec, (2) completely messes with the expected semantics of extends. So I think really, we should discourage additionalProperties: false to be configured (which I think is the status quo). If it was allowed, you'd want errors messaging if you try to extend from it or if you try to extend something else with this present in such a way as to lead to a situation where model properties can never practically be used. And it would only make sense as a decorator, not emitter-wide.

However (1) doesnt mess at all with these semantics. In fact, on both json schema and openapi emitter, there is a strong argument unevaluatedProperties should be by default "false":

You can imagine possibly this should be an emitter-level option that can override this default. As well as decorator overrides. But I think it makes sense to not have some implicit undeclared behaviour by default.