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.

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

Simplify uses of resolve-refs partial #1773

Merged
merged 6 commits into from Apr 9, 2024

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Mar 29, 2024

As discussed in #1749 (comment).

  • Since it is a recursing partial, we can call it once we get the data, and it will solve everything in the tree. In the end only the http-api shortcode was not doing that already.
  • Enable a lint in the OpenAPI validator to only allow $ref where it is supposed to be. It depends on Fix security schemes in OpenAPI definitions #1772 for the lint to pass.
  • Document our use of $ref inside examples to allow to compose them from other examples, although it is not supported by the OpenAPI and JSON Schema specs.
  • Fix a call to resolve-refs, even if it doesn't seem to actually be needed because calls to this shortcode point to schemas without $ref (which is why the bug was unnoticed).

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

@zecakeh zecakeh requested a review from a team as a code owner March 29, 2024 14:34
@@ -23,4 +23,7 @@
{{ $base_url := (index $api_data.servers 0).variables.basePath.default }}
{{ $path := delimit (slice "api" $spec $api) "/" }}

{{ partial "openapi/render-api" (dict "api_data" $api_data "base_url" $base_url "path" $path) }}
{{ $api_data = partial "json-schema/resolve-refs" (dict "schema" $api_data "path" $path) }}
{{ $api_data := partial "json-schema/resolve-allof" $api_data }}
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to call resolve-allof here now?

Copy link
Contributor Author

@zecakeh zecakeh Apr 9, 2024

Choose a reason for hiding this comment

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

Sorry, it must be a leftover of when I was trying stuff.

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 removed it.

Call it right after accessing the site.Data,
since it is recursing it will solve all references in the tree.
That way we don't need to wonder where to call it,
we trust the validators that the refs will be used in the right place.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@zecakeh
Copy link
Contributor Author

zecakeh commented Apr 9, 2024

Rebased on main to pass CI

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@richvdh
Copy link
Member

richvdh commented Apr 9, 2024

Rebased on main to pass CI

could you merge rather than rebase next time, please. Rebasing makes it hard to see what has changed since an earlier review.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@richvdh richvdh merged commit 2678370 into matrix-org:main Apr 9, 2024
12 checks passed
@zecakeh zecakeh deleted the resolve-refs-root branch April 9, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants