Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Present OpenAPI additionalProperties field #1162

Closed
wants to merge 8 commits into from
Closed

Present OpenAPI additionalProperties field #1162

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c69ac3b
Present OpenAPI `additionalProperties` field
Jul 4, 2022
9bc952e
Changelog
Jul 4, 2022
3398cf0
Use phrasing "Notes on additional properties"
Jul 21, 2022
1d26b36
Only italicise the boilerplate text
Jul 21, 2022
7da039a
Merge remote-tracking branch 'origin/main' into dmr/additional-proper…
Jul 21, 2022
dc8b133
Fix spelling of additionalProperties
DMRobertson Nov 22, 2022
6487e84
Describe `additionalProperties` in comments
Nov 22, 2022
87ded3b
Only include a table row for additionalProperties with a description
Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/client_server/newsfragments/1162.clarification
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use the `additionalProperties` field from the OpenAPI schemas to indicate that some JSON objects permit additional arbitrary key-value pairs.
2 changes: 1 addition & 1 deletion layouts/partials/json-schema/resolve-additional-types.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,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 "anchor" .anchor) }}
{{ return (dict "title" .title "properties" .properties "required" .required "enum" .enum "anchor" .anchor "additionalProperties" .additionalProperties) }}
{{ end }}
21 changes: 21 additions & 0 deletions layouts/partials/openapi/render-object-table.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
{{ $title := .title }}
{{ $properties := .properties}}
{{ $required := .required}}
{{ $additionalProperties := .additionalProperties }}
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

{{ if $properties }}

Expand Down Expand Up @@ -97,6 +98,26 @@

{{ end }}

{{/* Additional properties should either be an OpenAPI schema object, or a
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
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 )}}
<tr class="row-additional-properties">
<td colspan="3">
richvdh marked this conversation as resolved.
Show resolved Hide resolved
{{ if (and (reflect.IsMap $additionalProperties)) -}}
{{ if (index $additionalProperties "description") -}}
<em>Notes on additional properties:</em> {{ $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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@DMRobertson DMRobertson Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3398cf0 is an attempt at this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{{ end }}
</td>
</tr>
{{ end }}
</table>

{{ end }}