From c69ac3bb685ace4fcc1c939df65173ba000d43ac Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Jul 2022 21:24:56 +0100 Subject: [PATCH 1/7] Present OpenAPI `additionalProperties` field When looking at the "Authentication Data" type used in e.g. the account deactivation endpoint https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3accountdeactivate it was not clear to me that clients and servers should expect additional fields in this structure (the semantics of which are implied by the `type` of the authentication, or that of the previously established `session`.) In the OpenAPI spec, We occasionally mark some ojects as allowing arbitrary additional properties (`additionalProperties: true`, or `additionalProperties: { description: "..."}`). In other places we are more strict and provide a schema that additional properties must satisfy. In this PR we aim to make the first kind of additional properties (non-strict) more visible in the rendered spec. --- assets/scss/custom.scss | 4 ++++ .../json-schema/resolve-additional-types.html | 2 +- .../partials/openapi/render-object-table.html | 24 +++++++++++++++++++ layouts/partials/openapi/render-request.html | 2 +- 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/assets/scss/custom.scss b/assets/scss/custom.scss index 35b4e3dcc..3e4e44c80 100644 --- a/assets/scss/custom.scss +++ b/assets/scss/custom.scss @@ -370,6 +370,10 @@ footer { width: 75%; } + .row-additional-properties { + font-style: italic; + } + } pre { diff --git a/layouts/partials/json-schema/resolve-additional-types.html b/layouts/partials/json-schema/resolve-additional-types.html index 26df52e59..fa73c1bc2 100644 --- a/layouts/partials/json-schema/resolve-additional-types.html +++ b/layouts/partials/json-schema/resolve-additional-types.html @@ -88,5 +88,5 @@ but with (for example) different examples will be considered different. */}} {{ define "partials/clean-object" }} - {{ return (dict "title" .title "properties" .properties "required" .required "enum" .enum) }} + {{ return (dict "title" .title "properties" .properties "required" .required "enum" .enum "additionalProperties" .additionalProperties) }} {{ end }} diff --git a/layouts/partials/openapi/render-object-table.html b/layouts/partials/openapi/render-object-table.html index e132af309..6caca3756 100644 --- a/layouts/partials/openapi/render-object-table.html +++ b/layouts/partials/openapi/render-object-table.html @@ -14,6 +14,7 @@ {{ $caption := .caption }} {{ $properties := .properties}} {{ $required := .required}} +{{ $additionalProperties := .additionalProperties }} {{ if $properties }} @@ -93,6 +94,29 @@ {{ end }} + {{/* NB: the OpenAPI spec says "Consistent with JSON Schema, additionalProperties + defaults to true." Our schemas tend to omit additionalProperties; I think we + effectively assume it defaults to false. Therefore: we only output regarding + additionalProperties if it is explicitly set (i.e. is non-nil). */}} + {{ if (ne $additionalProperties nil )}} + + + {{ if (and (reflect.IsMap $additionalProperties)) -}} + {{ if (index $additionalProperties "description") -}} +May have additional properties: {{ $additionalProperties.description }} + {{ end }} + {{/* TODO: should we handle the case where additional properties are permitted, + but must follow an explicit schema? */}} + {{ else if $additionalProperties -}} +May have additional properties. + {{ else if (not $additionalProperties) -}} + {{/* At present we don't have any schema with additionalProperties: false. + But if we ever do, let's support it. */}} +May **not** have additional properties. + {{ end }} + + + {{ end }} {{ end }} diff --git a/layouts/partials/openapi/render-request.html b/layouts/partials/openapi/render-request.html index 5e8012996..345820f06 100644 --- a/layouts/partials/openapi/render-request.html +++ b/layouts/partials/openapi/render-request.html @@ -41,7 +41,7 @@

Request body

{{ $additional_types := partial "json-schema/resolve-additional-types" $schema }} {{ $additional_types = uniq $additional_types }} {{ range $additional_types }} - {{ partial "openapi/render-object-table" (dict "caption" .title "properties" .properties "required" .required) }} + {{ partial "openapi/render-object-table" (dict "caption" .title "properties" .properties "required" .required "additionalProperties" .additionalProperties) }} {{ end }}

Request body example

From 9bc952ef03caaba87a8b41b39f29735787dc27c8 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Jul 2022 21:38:43 +0100 Subject: [PATCH 2/7] Changelog --- changelogs/client_server/newsfragments/1162.clarification | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/client_server/newsfragments/1162.clarification diff --git a/changelogs/client_server/newsfragments/1162.clarification b/changelogs/client_server/newsfragments/1162.clarification new file mode 100644 index 000000000..e589f114d --- /dev/null +++ b/changelogs/client_server/newsfragments/1162.clarification @@ -0,0 +1 @@ +Use the `additionalProperties` field from the OpenAPI schemas to indicate that some JSON objects permit additional arbitrary key-value pairs. From 3398cf0f63f1f4bf6ba8a6637c3290da808e7440 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 21 Jul 2022 16:12:07 +0100 Subject: [PATCH 3/7] Use phrasing "Notes on additional properties" Ignore `true` values for additionalProperties. Error on `false` values. --- .../partials/openapi/render-object-table.html | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/layouts/partials/openapi/render-object-table.html b/layouts/partials/openapi/render-object-table.html index 6caca3756..e78dd901c 100644 --- a/layouts/partials/openapi/render-object-table.html +++ b/layouts/partials/openapi/render-object-table.html @@ -94,24 +94,21 @@ {{ end }} - {{/* NB: the OpenAPI spec says "Consistent with JSON Schema, additionalProperties - defaults to true." Our schemas tend to omit additionalProperties; I think we - effectively assume it defaults to false. Therefore: we only output regarding - additionalProperties if it is explicitly set (i.e. is non-nil). */}} + {{/* Additional properties should either be an OpenAPI schema object, or a + boolean. If it's the former, look for a `description` and render it as a note. + If it's `true`, do nothing. If it's `false`, raise a build-time error: + "Everything is extensible in Matrix unless otherwise noted". */}} {{ if (ne $additionalProperties nil )}} {{ if (and (reflect.IsMap $additionalProperties)) -}} {{ if (index $additionalProperties "description") -}} -May have additional properties: {{ $additionalProperties.description }} +Notes on additional properties: {{ $additionalProperties.description }} {{ end }} - {{/* TODO: should we handle the case where additional properties are permitted, - but must follow an explicit schema? */}} - {{ else if $additionalProperties -}} -May have additional properties. + {{/* TODO: should we handle the case where additional properties are + permitted, but must follow an explicit schema? */}} {{ else if (not $additionalProperties) -}} - {{/* At present we don't have any schema with additionalProperties: false. - But if we ever do, let's support it. */}} + {{ errorf "Unexpected additionalProperties=false for %s" $caption }} May **not** have additional properties. {{ end }} From 1d26b3637d99ca8a60f2f550c9c8f6a9ee3ec8ca Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 21 Jul 2022 16:13:09 +0100 Subject: [PATCH 4/7] Only italicise the boilerplate text I think this looks better, but what do I know? --- assets/scss/custom.scss | 4 ---- layouts/partials/openapi/render-object-table.html | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/assets/scss/custom.scss b/assets/scss/custom.scss index 3e4e44c80..35b4e3dcc 100644 --- a/assets/scss/custom.scss +++ b/assets/scss/custom.scss @@ -370,10 +370,6 @@ footer { width: 75%; } - .row-additional-properties { - font-style: italic; - } - } pre { diff --git a/layouts/partials/openapi/render-object-table.html b/layouts/partials/openapi/render-object-table.html index e78dd901c..5029f63c5 100644 --- a/layouts/partials/openapi/render-object-table.html +++ b/layouts/partials/openapi/render-object-table.html @@ -103,7 +103,7 @@ {{ if (and (reflect.IsMap $additionalProperties)) -}} {{ if (index $additionalProperties "description") -}} -Notes on additional properties: {{ $additionalProperties.description }} +Notes on additional properties: {{ $additionalProperties.description }} {{ end }} {{/* TODO: should we handle the case where additional properties are permitted, but must follow an explicit schema? */}} From dc8b133f2ecbe625ce177f72abcd515f6c970d22 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 22 Nov 2022 17:53:07 +0000 Subject: [PATCH 5/7] Fix spelling of additionalProperties Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- layouts/partials/openapi/render-object-table.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/layouts/partials/openapi/render-object-table.html b/layouts/partials/openapi/render-object-table.html index a3daa2076..f41d8ffd7 100644 --- a/layouts/partials/openapi/render-object-table.html +++ b/layouts/partials/openapi/render-object-table.html @@ -98,7 +98,7 @@ {{ end }} - {{/* Additional properties should either be an OpenAPI schema object, or a + {{/* additionalProperties should either be an OpenAPI schema object, or a boolean. If it's the former, look for a `description` and render it as a note. If it's `true`, do nothing. If it's `false`, raise a build-time error: "Everything is extensible in Matrix unless otherwise noted". */}} From 6487e84c4ec242485120f55cbb80c57e9690ea8b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 22 Nov 2022 18:12:26 +0000 Subject: [PATCH 6/7] Describe `additionalProperties` in comments --- .../partials/json-schema/resolve-additional-types.html | 1 + layouts/partials/openapi/render-object-table.html | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/layouts/partials/json-schema/resolve-additional-types.html b/layouts/partials/json-schema/resolve-additional-types.html index d56edbc8e..e0df56f49 100644 --- a/layouts/partials/json-schema/resolve-additional-types.html +++ b/layouts/partials/json-schema/resolve-additional-types.html @@ -15,6 +15,7 @@ * required * enum * anchor: a string suitable for using as an html anchor for this object (if `anchor_base` was set, and the object has a title) + * additionalProperties: either nil, a boolean, or an OpenAPI schema object (https://swagger.io/specification/#schema-object). Note that the returned array may contain duplicate objects. diff --git a/layouts/partials/openapi/render-object-table.html b/layouts/partials/openapi/render-object-table.html index f41d8ffd7..21a185e37 100644 --- a/layouts/partials/openapi/render-object-table.html +++ b/layouts/partials/openapi/render-object-table.html @@ -13,6 +13,15 @@ In some cases (such as response body specifications) this isn't used, and instead properties have a `required` boolean attribute. We support this too. + * `additionalProperties`: one of + + - `false` (no additional properties are permitted); + - `true` (additional properties pairs are permitted, with no constraints); + - an OpenAPI schema object [1] (additional properties are permitted, but + with constraints on `property_data`); or + - `nil` (not specified in schema, use `true` as a default value). + + [1]: https://swagger.io/specification/#schema-object */}} {{ $title := .title }} From 87ded3b25c97080ad4871cd2d10bf29c03bfd438 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 22 Nov 2022 18:31:17 +0000 Subject: [PATCH 7/7] Only include a table row for additionalProperties with a description --- .../partials/openapi/render-object-table.html | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/layouts/partials/openapi/render-object-table.html b/layouts/partials/openapi/render-object-table.html index 21a185e37..85597e142 100644 --- a/layouts/partials/openapi/render-object-table.html +++ b/layouts/partials/openapi/render-object-table.html @@ -107,26 +107,27 @@ {{ end }} - {{/* additionalProperties should either be an OpenAPI schema object, or a - boolean. If it's the former, look for a `description` and render it as a note. - If it's `true`, do nothing. If it's `false`, raise a build-time error: - "Everything is extensible in Matrix unless otherwise noted". */}} - {{ if (ne $additionalProperties nil )}} + {{/* If additionalProperties is true, do nothing. + If additionalProperties is nil, treat is as the default (true) and do nothing. + If additionalProperties is false, raise a build-time error: + "Everything is extensible in Matrix unless otherwise noted". + Otherwise, it should be an OpenAPI schema object. Include the description + of that schema object, if it exists. */}} + {{ if (and (ne $additionalProperties nil) (ne $additionalProperties true)) }} + {{ if (and (reflect.IsMap $additionalProperties)) -}} + {{ if (index $additionalProperties "description") -}} - {{ if (and (reflect.IsMap $additionalProperties)) -}} - {{ if (index $additionalProperties "description") -}} -Notes on additional properties: {{ $additionalProperties.description }} - {{ end }} - {{/* TODO: should we handle the case where additional properties are - permitted, but must follow an explicit schema? */}} - {{ else if (not $additionalProperties) -}} - {{ errorf "Unexpected additionalProperties=false for %s" $title }} -May **not** have additional properties. - {{ end }} + Notes on additional properties: {{ $additionalProperties.description }} - {{ end }} + {{ end }} + {{/* TODO: should we handle the case where additional properties are + permitted, but must follow an explicit schema? */}} + {{ else if (not $additionalProperties) -}} + {{ errorf "Unexpected additionalProperties=false for %s" $title }} + {{ end }} + {{ end }} {{ end }}