Skip to content

Simplify uses of resolve-refs partial#1773

Merged
richvdh merged 6 commits intomatrix-org:mainfrom
zecakeh:resolve-refs-root
Apr 9, 2024
Merged

Simplify uses of resolve-refs partial#1773
richvdh merged 6 commits intomatrix-org:mainfrom
zecakeh:resolve-refs-root

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

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

zecakeh added 5 commits April 9, 2024 18:50
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 zecakeh force-pushed the resolve-refs-root branch from 278446d to dcc33eb Compare April 9, 2024 16:52
@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
@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.

2 participants