Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Fix support for using $ref in a component definition #503

Merged
merged 3 commits into from
Jun 24, 2020
Merged

Conversation

kylef
Copy link
Member

@kylef kylef commented Jun 23, 2020

I think it might be a bit hard for a reviewer to get their head around some of these changes as the referencing logic is some of the most complicated code in the project, so I'll try do my best at explaining these changes.

The following OpenAPI 3 document was not being parsed correctly:

openapi: 3.0.0
info:
  title: example
  version: 1.0.0
paths: {}
components:
  schemas:
    User:
      type: object
    UserAlias:
      $ref: '#/components/schemas/User'

Support for using $ref directly in a reusable component against another reusable component wasn't supported. If users did that, they'd get incorrect behaviour and possibly incorrect errors (in the case for parameters which have required properties which a $ref object wouldn't contain).

To fix this I've made 3 changes which are broken into separate commits:

  1. Before parsing, we create internal state of all the possible schemas. This allows us to parse schema A which references schema B (which isn't parsed yet), it's also part of supporting circular references. This is done by storing context.state.components.set('schemas', ...) where there is a collection of member elements with all the nessecery schema names. That way we can validate schemas exist. This particular code was specific to schemas only, in the first commit I have made this generic to apply to all reusable components. This internal state is needed to support references in other components.
  2. Updated parseReferenceObject (which has dereferencing capabilities) to support multi-level referencing. This function supports returning "dereferenced" elements, this is used in areas such as parseParametersObject where parameter validation against a href uses the dereferenced parameter to get hold of the 'name'. Without this change, the function would return an element such as {"element": "LimitAlias"} instead the dereferenced limit parameter such as {"element": "member", ...}.
  3. This is the core part of the fix, which is updating the logic that parses each component member to support referencing via the existing parseReference function. I've had to add the argument returnReferenceElement of parseReferenceObject to parseReference so it can be used here. Since we do actually want a referenced element back instead of a dereference element in this case (which isn't common with the rest of the uses of parseReference).

The last commit also contains integration tests to ensure this works end-to-end (for example JSON message body is created from the reference, etc). I want to ensure it works at all levels.

@kylef kylef added bug Something isn't working openapi3 labels Jun 23, 2020
@kylef kylef requested review from opichals and pksunkara June 23, 2020 19:48
@kylef
Copy link
Member Author

kylef commented Jun 23, 2020

This is related to apiaryio/dredd#1762, the underlying document on this issue makes use of this feature of OpenAPI 3.0.

@kylef
Copy link
Member Author

kylef commented Jun 24, 2020

I think there’s a case where this logic will get into an infinite loop, should be simple to resolve so adding this step

  • Handle circular reference in the dereference function in parseReferenceObject

Copy link
Contributor

@opichals opichals left a comment

Choose a reason for hiding this comment

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

Looks good to me. One nit to consider.

(object) => {
const contextMember = context.state.components.getMember(member.key.toValue());

if (contextMember) {
contextMember.value = object;
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

For review, this branch becomes dead code due to the first commit we only ever have the first case. This was evident in code coverage going down due to this line no longer being called/tested.

Copy link
Contributor

@opichals opichals left a comment

Choose a reason for hiding this comment

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

🚀

@kylef kylef merged commit ab08f52 into master Jun 24, 2020
@kylef kylef deleted the kylef/ref branch June 24, 2020 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working openapi3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants