Present OpenAPI `additionalProperties` field #1162

DMRobertson · 2022-07-04T20:36:57Z

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.

See https://pr1162--matrix-spec-previews.netlify.app/client-server-api/#post_matrixclientv3accountdeactivate

Preview: https://pr1162--matrix-spec-previews.netlify.app

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.

turt2live

generally this seems fine, though there seems to be a bit of confusion around what clients are able to send (which is anything it feels like - servers don't have to listen though)

turt2live · 2022-07-05T19:52:44Z

layouts/partials/openapi/render-object-table.html

+        {{ 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.


Everything is extensible in Matrix unless otherwise noted, so this is a bit misleading.

I don't follow---what do you find misleading?

The whole sentence :D

Additional properties are always allowed, even if not advertised by the spec. The server just isn't required to do anything with them (unless it's dealing with signatures)

Okay. Then perhaps this can be changed so that

we don't do anything if additionalProperties: true

we raise a build-time error if additionalProperties: false

Rather than "May have additional properties:" in the generated HTML, we write "Notes on additional properties:".

How does that sound?

seems reasonable, assuming additionalProperties can be assigned a boolean (I don't think it can?)

See https://swagger.io/specification/#schema-object

The following properties are taken from the JSON Schema definition but their definitions were adjusted to the OpenAPI Specification.

additionalProperties - Value can be boolean or object. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. Consistent with JSON Schema, additionalProperties defaults to true.

And for completeness: the original JSON schema additionalProperties are defined here.

3398cf0 is an attempt at this.

@turt2live, @DMRobertson: I'm not sure I'm following the discussion here. Can I just check that everyone is clear what we're discussing?

The question is around the correct behaviour if we encounter an object schema where additionalProperties is explictly set to false (as opposed to leaving it unset, which is equivalent to setting it to true).

Earlier drafts of JSON Schema give a clear definition of what that means:

If "additionalProperties" is false, validation succeeds only if the instance is an object and all properties on the instance were covered by "properties" and/or "patternProperties".

In other words: this is explicitly about trying to define an object which may not be extended.

I don't entirely object to the decision to emit an error message, if we think it's somehow un-matrixy to try to define an object that isn't allowed any additional properties. I'd just like to be sure that we made that decision with good information.

We actually have that case (exactly one, unless I missed some weirdly formatted), and it's in m.receipt events: . additionalProperties: false is used next to patternProperties to forbid anything that does not follow the pattern. Which is quite valid and smart, in my opinion. There's a big question however on what the homeserver is supposed to do with this boundary. I would treat it as a "MUST ignore" of sorts, whereas omitting additionalProperties entirely would correspond to "MAY ignore". Does it make sense?

Ignore `true` values for additionalProperties. Error on `false` values.

I think this looks better, but what do I know?

DMRobertson · 2022-07-21T15:16:09Z

Oh. Looks like I have a merge to resolve.

…ties

layouts/partials/openapi/render-object-table.html

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh · 2022-11-30T12:45:03Z

layouts/partials/openapi/render-object-table.html

+      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)) -}}


redundant and here.

richvdh · 2022-11-30T12:48:56Z

layouts/partials/openapi/render-object-table.html

+ {{ if (and (ne $additionalProperties nil) (ne $additionalProperties true)) }}
+  {{ if (and (reflect.IsMap $additionalProperties)) -}}


can we restructure these if statements to reduce the nesting?

{{ if (or (eq $additionalProperties nil) (eq $additionalProperties true)) }} {{/* do nothing */}} {{ else if (reflect.IsMap $additionalProperties) -}} ... {{ else if (not $additionalProperties) -}} ... {{ end }}

or something.

richvdh · 2022-11-30T12:50:08Z

layouts/partials/openapi/render-object-table.html

+  {{/* 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 }}


it's unclear to me that we should be raising an error in this case. Isn't it legitimate to specify that a given object may not have additional properties, even if that is currently unused?

🤷 There's some context here: #1162 (comment) ; I don't have any strong opinions here.

richvdh · 2023-10-03T21:17:42Z

@DMRobertson are you still interested in pursuing this? Anything we can do to unblock you?

DMRobertson · 2023-10-25T13:52:09Z

It'd be nice to do this at some point, but I don't think I'm going to find the time. Let's close this off for now.

David Robertson added 2 commits July 4, 2022 21:24

Changelog

9bc952e

DMRobertson marked this pull request as ready for review July 4, 2022 20:39

DMRobertson requested a review from a team as a code owner July 4, 2022 20:39

turt2live reviewed Jul 5, 2022

View reviewed changes

DMRobertson marked this pull request as draft July 14, 2022 19:07

David Robertson added 2 commits July 21, 2022 16:12

Use phrasing "Notes on additional properties"

3398cf0

Ignore `true` values for additionalProperties. Error on `false` values.

Only italicise the boilerplate text

1d26b36

I think this looks better, but what do I know?

DMRobertson requested a review from turt2live July 21, 2022 15:15

DMRobertson marked this pull request as ready for review July 21, 2022 15:15

Merge remote-tracking branch 'origin/main' into dmr/additional-proper…

7da039a

…ties

DMRobertson mentioned this pull request Aug 15, 2022

Moving towards stronger schemas for response types matrix-org/synapse#13529

Open

DMRobertson mentioned this pull request Aug 25, 2022

/createRoom returns spurious alias property matrix-org/synapse#6879

Closed

DMRobertson mentioned this pull request Oct 13, 2022

OneTimeKeys is not defined by the CS API spec #1287

Open

DMRobertson mentioned this pull request Oct 21, 2022

S-S API does not define User Devices struct. #1300

Open

richvdh requested a review from a team November 22, 2022 16:58

richvdh requested changes Nov 22, 2022

View reviewed changes

layouts/partials/openapi/render-object-table.html Show resolved Hide resolved

layouts/partials/openapi/render-object-table.html Outdated Show resolved Hide resolved

layouts/partials/openapi/render-object-table.html Show resolved Hide resolved

DMRobertson and others added 3 commits November 22, 2022 17:53

Fix spelling of additionalProperties

dc8b133

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Describe additionalProperties in comments

6487e84

Only include a table row for additionalProperties with a description

87ded3b

DMRobertson requested review from richvdh and removed request for turt2live November 22, 2022 18:45

richvdh requested changes Nov 30, 2022

View reviewed changes

richvdh mentioned this pull request May 8, 2023

Clarify signatures upload endpoint #1495

Merged

DMRobertson closed this Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Present OpenAPI `additionalProperties` field #1162

Present OpenAPI `additionalProperties` field #1162

DMRobertson commented Jul 4, 2022 •

edited

Loading

turt2live left a comment

turt2live Jul 5, 2022

DMRobertson Jul 11, 2022

turt2live Jul 11, 2022

DMRobertson Jul 14, 2022

turt2live Jul 14, 2022

DMRobertson Jul 14, 2022 •

edited

Loading

DMRobertson Jul 14, 2022

DMRobertson Jul 21, 2022

richvdh Nov 30, 2022

KitsuneRal Mar 1, 2023

DMRobertson commented Jul 21, 2022

richvdh Nov 30, 2022

richvdh Nov 30, 2022

richvdh Nov 30, 2022

DMRobertson Nov 30, 2022

richvdh commented Oct 3, 2023

DMRobertson commented Oct 25, 2023

		{{ if (and (ne $additionalProperties nil) (ne $additionalProperties true)) }}
		{{ if (and (reflect.IsMap $additionalProperties)) -}}

Present OpenAPI additionalProperties field #1162

Present OpenAPI additionalProperties field #1162

Conversation

DMRobertson commented Jul 4, 2022 • edited Loading

turt2live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMRobertson Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMRobertson commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Oct 3, 2023

DMRobertson commented Oct 25, 2023

Present OpenAPI `additionalProperties` field #1162

Present OpenAPI `additionalProperties` field #1162

DMRobertson commented Jul 4, 2022 •

edited

Loading

DMRobertson Jul 14, 2022 •

edited

Loading