Upgrade Swagger data to OpenAPI 3.1 #1310

zecakeh · 2022-10-28T09:13:42Z

This is still a rough draft.

What was done to get here:

Ran a custom script that uses swagger2openapi to upgrade to OpenAPI 3.0 in batch, but keeps the comments at the top (so the license).
Made a few edits manually to follow the migration guide between OpenAPI 3.0 and 3.1.
Updated and ran scripts/validator.js on the folders in data/api and fixed any issue that was reported.
The converter merged the server URLs into an array of servers. I edited them to split the URLs into variables so we can access the basePath easily for the other tools.
Built the spec before the upgrade, and then again after the upgrade, and fixed the differences between the HTML files. Most of those needed either:
- Editing the templates to make them compatible with the new format,
- Restoring data that was removed by the converter (some examples and fields at the same level as a $ref).
Updated scripts/dump-swagger.py to adapt to the changes in the data and generate valid OpenAPI 3.1 data.
Added scripts/swagger-preview.html to allow to preview the generated API with RapiDoc, and to make sure that it works. Also updated the README to replace instructions for previewing with Swagger UI (which is not compatible with OpenAPI 3.1).

What's left to do:

The converter added a lot of unnecessary newlines in the descriptions, they should probably be cleaned up.
Most of the JSON examples were converted to YAML. Maybe convert all the examples to YAML? Or revert this to make the diff smaller?
4 files do not pass validation in data/api/server-server:
- keys_query.yaml and keys_server.yaml: the keyId property in the path cannot be required: false, in the OpenAPI 3.1 spec, all properties in path must be required: true.
- invites-v1.yaml and invites-v2.yaml: the validator complains that it cannot resolve the reference in the example of the 200 response. It's not true because the same reference is used at another place in the file without any issue, so there must be something wrong with the syntax around the $ref.
Check every single file that no data was lost (including comments)

I'll open discussions on the code for feedback for the remaining tasks.

Closes #331.

Signed-off-by: Kévin Commaille [email protected]

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

data/api/application-service/definitions/location.yaml

data/api/application-service/query_room.yaml

data/api/client-server/admin.yaml

data/api/server-server/invites-v1.yaml

data/api/server-server/invites-v2.yaml

data/api/server-server/keys_query.yaml

data/api/server-server/keys_server.yaml

data/api/client-server/appservice_room_directory.yaml

richvdh · 2022-11-08T21:12:20Z

I'm going to mark this as "ready for review" to gather some feedback. It's likely to get overlooked while it's in draft.

richvdh · 2022-11-08T21:53:31Z

@zecakeh thank you so much for picking this up - there is a lot of great work here and I am very excited to see it land!

I've made a few comments in reply to your questions.

Generally, I'd love it if we could find ways to pull out bits of this PR to reduce the size of it. For example:

Added scripts/swagger-preview.html to allow to preview the generated API with RapiDoc, and to make sure that it works. Also updated the README to replace instructions for previewing with Swagger UI (which is not compatible with OpenAPI 3.1).

Given RapiDoc is compatible with both OpenAPI 2 and OpenAPI 3, let's do this now as a separate PR, before we change anything else?
If there are changes that we can make to the existing definitions (such as replacing |- with >- for the descriptions) which will make them easier to convert, let's do that?

Another question: How much manual editing did you do? Should we aim to script the changes? It might be easier to review a set of conversion scripts than 167 manually-edited files!

zecakeh · 2022-11-09T09:33:50Z

Thanks for the feedback.

  Given RapiDoc is compatible with both OpenAPI 2 and OpenAPI 3, let's do this now as a separate PR, before we change anything else?

Done in #1331.

* If there are changes that we can make to the existing definitions (such as replacing `|-` with `>-` for the descriptions) which will make them easier to convert, let's do that?

Sure, I'll look into it.

Another question: How much manual editing did you do? Should we aim to script the changes? It might be easier to review a set of conversion scripts than 167 manually-edited files!

I think I did more manual editing than necessary, mostly because I didn't know what I was getting into, and what a valid result would look like. I'll look into writing scripts to automate most (or all) of it.

zecakeh · 2022-11-10T10:55:31Z

I split the different steps needed for conversion, between automatic and manual steps. I also removed the changes that are not strictly necessary.

With these commits:

The definitions pass the validator.js script.
The generated spec is unchanged except for the same 4 definitions files that wouldn't pass validation otherwise.

I found out another small thing that could reduce the diff if done in a separate PR: switching the definitions and tools to use x-servers. Then during conversion it would just be a x-servers servers change.

zecakeh · 2022-11-10T11:54:55Z

I tried with another validator (namely Redocly CLI) and it complained about a few things.

I trust these changes because most of them are required by swagger-parser when declaring the files as OpenAPI 3.0.0, but it stops complaining for some reason when the version is OpenAPI 3.1.0. They also make sense.

Redocly CLI also complains about $ref in examples which doesn't seem to be actually supported by the OpenAPI spec: it expects either the whole example as a $ref, or the complete content of the example.

richvdh · 2022-11-15T16:39:58Z

Redocly CLI also complains about $ref in examples which doesn't seem to be actually supported by the OpenAPI spec: it expects either the whole example as a $ref, or the complete content of the example.

Ok. As I understand it, there are only a couple of places where this is a problem? If so, let's fix it by just copying the complete content into the right place instead of using $ref. Could you open a separate PR?

Likewise, would you be able to open a separate PR for getting rid of {keyId} on the /keys endpoints?

scripts/package.json

zecakeh · 2022-11-17T17:15:48Z

Redocly CLI also complains about $ref in examples which doesn't seem to be actually supported by the OpenAPI spec: it expects either the whole example as a $ref, or the complete content of the example.

Ok. As I understand it, there are only a couple of places where this is a problem? If so, let's fix it by just copying the complete content into the right place instead of using $ref. Could you open a separate PR?

Done in #1349.

Likewise, would you be able to open a separate PR for getting rid of {keyId} on the /keys endpoints?

Done in #1350.

data/api/server-server/user_keys.yaml

richvdh · 2022-12-20T23:29:46Z

@zecakeh it's gone a bit quiet here, so I just wanted to say I'm still very excited about seeing this land. I'm trying to push through MSC3938 so we can fix the /keys endpoints, and I believe @KitsuneRal is working his way through a more comprehensive review on this. Please bear with us!

KitsuneRal

Starting slow: please see some comments from reviewing application-service/, meanwhile I'm in for CS API (but that will likely be my Christmas reading).

data/api/application-service/protocols.yaml

KitsuneRal

A few more from the first part of client-server/

data/api/client-server/administrative_contact.yaml

data/api/client-server/appservice_room_directory.yaml

data/api/client-server/capabilities.yaml

data/api/client-server/content-repo.yaml

KitsuneRal

Okay, client-server/ done.

I realise that the whole body will probably have to be re-converted from the most recent API definitions, so applying those suggestions now would not be prudent - but at least those can be used to apply changes after (or even before, like adding missing http schemes).

data/api/client-server/create_room.yaml

data/api/client-server/filter.yaml

data/api/client-server/inviting.yaml

KitsuneRal · 2023-01-10T12:52:16Z

data/api/client-server/keys.yaml

-                # $ref: "definitions/one_time_keys.yaml"
-                # XXX: We can't define an actual object here, so we have to hope
-                # that people will look at the swagger source or can figure it out
-                # from the other endpoints/example.


AFAIR, this can be reconsidered now that we have OpenAPI v3 expressive powers - namely, definitions/one_time_keys.yaml is written with OpenAPI v3 in mind.

data/api/client-server/sync.yaml

data/api/client-server/third_party_membership.yaml

data/api/client-server/typing.yaml

data/api/client-server/wellknown.yaml

KitsuneRal

The rest except server-server is covered. Got a couple of ideas after reviewing the scripts - please find below.

data/api/identity/v2_associations.yaml

scripts/package.json

scripts/openapi-convert/object-fixes.mjs

scripts/openapi-convert/string-fixes.mjs

scripts/openapi-convert/convert-definitions.mjs

KitsuneRal

The last piece, server-server/ completed. This one has been more problematic - aside from things I could debug (see the comments), most concerning is that the conversion has apparently lost a few examples. I guess it might be because of some weird example/examples confusion or the wrong level of nesting but didn't dig into this, honestly.

data/api/server-server/backfill.yaml

data/api/server-server/keys_query.yaml

data/api/server-server/invites-v1.yaml

data/api/server-server/invites-v2.yaml

data/api/server-server/joins-v1.yaml

data/api/server-server/third_party_invite.yaml

data/api/server-server/transactions.yaml

data/api/server-server/user_devices.yaml

KitsuneRal · 2023-01-17T17:44:35Z

What I would suggest after figuring out what to do on each item from the laundry lists above:

apply preparatory commits where necessary (maybe even take them out to separate PRs, as before); I suspect we got examples defined in the wrong place or form in server-server/, fixing that looks like one case here.
re-apply the conversion over the most recent main
apply all the manual post-fixes
force-push the branch under the same name
I review the changes across the force-push (GitHub allows to do that)
we plan follow-up fixes if any remain

zecakeh · 2023-01-24T17:09:25Z

apply preparatory commits where necessary (maybe even take them out to separate PRs, as before); I suspect we got examples defined in the wrong place or form in server-server/, fixing that looks like one case here.

Thanks for taking the time to review everything, I'll try to open PRs for those in the next days.

zecakeh · 2023-02-26T10:40:12Z

So we should be good to re-apply the upgrade after #1454 is merged and we agree on the correct fix for server-server/user_devices.yaml (#1310 (comment)), unless I forgot something else that can be fixed before the upgrade.

scripts/openapi-convert/object-fixes.mjs

KitsuneRal · 2023-03-08T22:33:47Z

Those PRs are all merged - we should be good to go?

zecakeh · 2023-03-09T19:15:25Z

Upgrade re-done on main.

I have added back all comments and examples that were gone. I checked that it passed two OpenAPI validators (swagger-parser and redocly-cli) and that the generated spec has no unwanted changes.

Before merging this we probably want to revert the commit adding the conversion scripts, but I thought it would be better if they were part of the branch history.

KitsuneRal

Sorry for the delayed review. Things are mostly in order now, save for one place with the user agent string sliced into parts in some weird (and actually not correct) way, and examples being kind-of-but-not-quite JSON now. I suggest we go forward with the currently formatted examples, but please address the first comment. Then (after yet another rebase... sorry for that) it should be good enough for merging.

data/api/client-server/admin.yaml

zecakeh · 2023-05-08T09:38:22Z

Should I wait until #1495 is merged to rebase, to avoid a conflict?

uhoreg · 2023-05-09T02:22:50Z

Should I wait until #1495 is merged to rebase, to avoid a conflict?

It has been merged, so should be safe to rebase. 😉

Signed-off-by: Kévin Commaille <[email protected]>

This reverts commit 667c39f.

zecakeh · 2023-05-09T16:54:26Z

Rebased on latest main.

I was unhappy to leave the examples as is so I wrote a dirty little JS script that converts them back to JSON. Hopefully that reduces the diff.

KitsuneRal · 2023-06-07T00:28:57Z

Thanks for your patience, again. I think it is as good as it can be for now. The remaining comments can be addressed later. @matrix-org/spec-core-team, I guess it's quite a good moment to merge this, while Matrix v1.8 is still kind of away and there's still time to iron parts that still stick out?

zecakeh · 2023-06-07T07:47:14Z

So I should rebase one last time to resolve the conflicts so it can be merged?

richvdh · 2023-06-07T09:36:30Z

So I should rebase one last time to resolve the conflicts so it can be merged?

yes please - either rebase, or better yet, just merge main into your branch and fix up the conflicts by hand, so the changes since @KitsuneRal's review are easy to see. Once you've done so I will press the merge button.

KitsuneRal · 2023-06-07T09:48:37Z

I'm not sure merging will work here because the PR is effectively generated every time. If there's any new endpoint (and I think there was some over this month) it has to be converted by the script.

Up to the hero to decide though.

zecakeh · 2023-06-07T11:23:01Z

I improved the script to be able to run it on a single file at a time so I was able to make a merge commit. Once again I checked that they pass the validation and that the generated Matrix spec doesn't have any changes.

richvdh

🎉

richvdh · 2023-06-07T12:17:36Z

Thank you so so much to @zecakeh for all your hard work on this, and to @KitsuneRal for reviewing it. It's fantastic to finally have this landed

zecakeh commented Oct 28, 2022

View reviewed changes

zecakeh mentioned this pull request Oct 28, 2022

Upgrade OpenAPI #331

Closed

richvdh marked this pull request as ready for review November 8, 2022 21:12

richvdh requested a review from a team as a code owner November 8, 2022 21:12

zecakeh mentioned this pull request Nov 9, 2022

Update instructions to preview Swagger definitions #1331

Merged

zecakeh force-pushed the update-openapi branch from a0bc100 to 399755c Compare November 12, 2022 08:43

richvdh requested changes Nov 15, 2022

View reviewed changes

scripts/package.json Show resolved Hide resolved

anoadragon453 mentioned this pull request Nov 15, 2022

The 'Content' section of m.receipt is empty #1343

Closed

This was referenced Nov 17, 2022

Include examples inline instead of using a reference for invite endpoint definitions #1349

Merged

Remove keyId from the server keys endpoints #1350

Merged

zecakeh mentioned this pull request Nov 17, 2022

Fix POST _matrix/federation/v1/user/keys/claim response schema #1351

Merged

zecakeh commented Nov 17, 2022

View reviewed changes

data/api/server-server/user_keys.yaml Outdated Show resolved Hide resolved

dylhack mentioned this pull request Dec 1, 2022

replace "<content>" with "content" #1370

Merged

KitsuneRal reviewed Dec 21, 2022

View reviewed changes

data/api/application-service/protocols.yaml Outdated Show resolved Hide resolved

data/api/application-service/protocols.yaml Show resolved Hide resolved

data/api/application-service/protocols.yaml Show resolved Hide resolved

KitsuneRal requested changes Dec 28, 2022

View reviewed changes

KitsuneRal reviewed Jan 10, 2023

View reviewed changes

KitsuneRal reviewed Jan 14, 2023

View reviewed changes

KitsuneRal requested changes Jan 15, 2023

View reviewed changes

zecakeh mentioned this pull request Feb 26, 2023

Remove overridden references to examples/minimal_pdu.json and add the missing room_id field. #1454

Merged

KitsuneRal reviewed Mar 1, 2023

View reviewed changes

scripts/openapi-convert/object-fixes.mjs Outdated Show resolved Hide resolved

zecakeh force-pushed the update-openapi branch from 399755c to 8c3e424 Compare March 9, 2023 19:12

KitsuneRal reviewed May 7, 2023

View reviewed changes

data/api/client-server/admin.yaml Outdated Show resolved Hide resolved

data/api/client-server/admin.yaml Outdated Show resolved Hide resolved

zecakeh added 7 commits May 9, 2023 15:39

Create Swagger 2.0 to OpenAPI 3.1 conversion scripts

667c39f

Signed-off-by: Kévin Commaille <[email protected]>

Automatic conversion of API files to OpenAPI 3.1.0

10603b4

Signed-off-by: Kévin Commaille <[email protected]>

Manual fixes to API files

c3a4f5b

Signed-off-by: Kévin Commaille <[email protected]>

Update scripts for OpenAPI 3.1

4fafc98

Signed-off-by: Kévin Commaille <[email protected]>

Update layouts for OpenAPI 3.1

02f2c47

Signed-off-by: Kévin Commaille <[email protected]>

Add newsfragment

902e381

Signed-off-by: Kévin Commaille <[email protected]>

Revert "Create Swagger 2.0 to OpenAPI 3.1 conversion scripts"

e2efbfb

This reverts commit 667c39f.

zecakeh force-pushed the update-openapi branch from 8c3e424 to e2efbfb Compare May 9, 2023 16:52

KitsuneRal approved these changes Jun 7, 2023

View reviewed changes

Merge branch 'main' into update-openapi

e72212b

richvdh approved these changes Jun 7, 2023

View reviewed changes

richvdh merged commit 45b6aaf into matrix-org:main Jun 7, 2023

zecakeh deleted the update-openapi branch June 7, 2023 12:23

This was referenced Jun 8, 2023

Use Redocly CLI to validate OpenAPI definitions #1558

Merged

Fix definition of response of POST /_matrix/federation/v1/user/keys/claim #1559

Merged

zecakeh mentioned this pull request Jun 22, 2023

Fix schema of custom fields in query for appservice API #1584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Swagger data to OpenAPI 3.1 #1310

Upgrade Swagger data to OpenAPI 3.1 #1310

zecakeh commented Oct 28, 2022 •

edited by github-actions bot

Loading

richvdh commented Nov 8, 2022

richvdh commented Nov 8, 2022

zecakeh commented Nov 9, 2022

zecakeh commented Nov 10, 2022

zecakeh commented Nov 10, 2022 •

edited

Loading

richvdh commented Nov 15, 2022

zecakeh commented Nov 17, 2022

richvdh commented Dec 20, 2022

KitsuneRal left a comment

KitsuneRal left a comment

KitsuneRal left a comment

KitsuneRal Jan 10, 2023

KitsuneRal left a comment

KitsuneRal left a comment

KitsuneRal commented Jan 17, 2023

zecakeh commented Jan 24, 2023

zecakeh commented Feb 26, 2023 •

edited

Loading

KitsuneRal commented Mar 8, 2023

zecakeh commented Mar 9, 2023 •

edited

Loading

KitsuneRal left a comment

zecakeh commented May 8, 2023

uhoreg commented May 9, 2023

zecakeh commented May 9, 2023

KitsuneRal commented Jun 7, 2023

zecakeh commented Jun 7, 2023 •

edited

Loading

richvdh commented Jun 7, 2023

KitsuneRal commented Jun 7, 2023 •

edited

Loading

zecakeh commented Jun 7, 2023

richvdh left a comment

richvdh commented Jun 7, 2023

Upgrade Swagger data to OpenAPI 3.1 #1310

Upgrade Swagger data to OpenAPI 3.1 #1310

Conversation

zecakeh commented Oct 28, 2022 • edited by github-actions bot Loading

What was done to get here:

What's left to do:

richvdh commented Nov 8, 2022

richvdh commented Nov 8, 2022

zecakeh commented Nov 9, 2022

zecakeh commented Nov 10, 2022

zecakeh commented Nov 10, 2022 • edited Loading

richvdh commented Nov 15, 2022

zecakeh commented Nov 17, 2022

richvdh commented Dec 20, 2022

KitsuneRal left a comment

Choose a reason for hiding this comment

KitsuneRal left a comment

Choose a reason for hiding this comment

KitsuneRal left a comment

Choose a reason for hiding this comment

KitsuneRal Jan 10, 2023

Choose a reason for hiding this comment

KitsuneRal left a comment

Choose a reason for hiding this comment

KitsuneRal left a comment

Choose a reason for hiding this comment

KitsuneRal commented Jan 17, 2023

zecakeh commented Jan 24, 2023

zecakeh commented Feb 26, 2023 • edited Loading

KitsuneRal commented Mar 8, 2023

zecakeh commented Mar 9, 2023 • edited Loading

KitsuneRal left a comment

Choose a reason for hiding this comment

zecakeh commented May 8, 2023

uhoreg commented May 9, 2023

zecakeh commented May 9, 2023

KitsuneRal commented Jun 7, 2023

zecakeh commented Jun 7, 2023 • edited Loading

richvdh commented Jun 7, 2023

KitsuneRal commented Jun 7, 2023 • edited Loading

zecakeh commented Jun 7, 2023

richvdh left a comment

Choose a reason for hiding this comment

richvdh commented Jun 7, 2023

zecakeh commented Oct 28, 2022 •

edited by github-actions bot

Loading

zecakeh commented Nov 10, 2022 •

edited

Loading

zecakeh commented Feb 26, 2023 •

edited

Loading

zecakeh commented Mar 9, 2023 •

edited

Loading

zecakeh commented Jun 7, 2023 •

edited

Loading

KitsuneRal commented Jun 7, 2023 •

edited

Loading