Skip to content

Improve type safety with restricted component references #1991

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

Closed
wants to merge 2 commits into from

Conversation

vearutop
Copy link
Contributor

JSON Schema does not define a concept of $ref usage in data values, neither it provides a way to ensure if resolved data value is valid. Nevertheless using $ref in data is popular and is used in OpenAPI.

If it is not possible to validate the resolved value, we can at least have best effort validation that reference leads to a correct place.

OpenAPI spec has a pre-defined place to store some kinds of referenced values: components.

For example responses are defined as follows:

      responses:
        type: object
        patternProperties:
          '^[a-zA-Z0-9\.\-_]+$':
            oneOf:
              - $ref: '#/definitions/Reference'
              - $ref: '#/definitions/Response'

The problem here is that current schema allows $ref to lead to any place, not only to #/components/responses/....

This PR introduces restricted local references of schema components, like for example ResponseReference:

  ResponseReference:
    type: object
    required:
      - $ref
    patternProperties:
      '^\$ref$':
        type: string
        format: uri-reference
        oneOf:
          - pattern: '^#/components/responses/'
          - not:
              pattern: '^#/'

External references are not affected, they still can point to any place.
Also it is possible to avoid restriction by using absolute URI to a local fragment.

This PR targets to invalidate semantically incorrect (but valid) schemas with confused references, example:

paths:
  /pets:
    get:
      parameters:
        - $ref: "#/components/responses/UnexpectedError"

@MikeRalphson
Copy link
Member

This PR (while very welcome) seems to introduce a restriction in the JSON schema which is not mandated by the matching markdown source-of-truth specification.

My main reservation about adopting this restriction would be around the representation of OpenAPI documents where external $refs have been resolved but internal $refs still remain. I know of at least one tool (disclosure: which I wrote) which will populate the first $ref pointing at an external resource with a resolved copy of that target, but subsequent instances of that $ref value with a new direct internal $ref to the first location. This is not guaranteed to be within #/components/.

If the fully qualified URL to the OpenAPI document is not fixed or known, or the resultant document is required to remain portable, then I don't believe using a FQ URL prefix for the $ref is a viable workaround.

If we were to adopt it, there would be the question of whether it is a semver breaking change. I cannot see that we can simply merge it into master.

@handrews
Copy link
Member

@MikeRalphson one thing to keep in mind as we consider moving to the next JSON Schema draft for OAS 3.1 (or 4.0) is how $id might fit in. Note that I put up a PR two days ago that makes $id a lot more straightforward by making it do a lot less. That removes the problem where you could very easily write $id values with undefined behavior.

Of course, OAS could still restrict the usage of $id, or add these restrictions to 3.0.x and reconsider them in 3.1, I'm just trying to make sure we don't back ourselves into a corner.

@MikeRalphson
Copy link
Member

Thanks @handrews could you link to the PR for the wider audience?

@handrews
Copy link
Member

handrews commented Aug 19, 2019

@MikeRalphson certainly! I'm never sure how much people want to dive into works-in-progress.

The PR is json-schema-org/json-schema-spec#780, which is a change relative to the work-in-progress-review copy of the core spec (see also other WIP documents).

The PRs that are currently open are the last known mandatory PRs for the new draft. I am posting one more for consideration that will only go in if there is fairly quick agreement, otherwise it can be punted to the next draft (it addresses the kind of complicated CREF notes about $ref that are in the PR I linked).

If the reviews go smoothly this week, we can hopefully publish next week.

@handrews
Copy link
Member

NOTE: I put the wrong WIP core spec link in the previous comment, it's now been updated. But if you clicked it from a notification email it sent you to the wrong place. Sorry about that.

@webron
Copy link
Member

webron commented Aug 19, 2019

I'm inclined to agree with @MikeRalphson. While it is not logical to put internal references that point to the 'wrong' component, the specification does not prohibit it and so neither should the JSON Schema for it.

The PR also seem to restrict internal references to internal non-component locations such as extensions, or even just another operation, both of which are valid use cases (not encouraged, but valid).

@vearutop
Copy link
Contributor Author

Thank you for feedback, I agree such restriction can be considered a breaking change.

It seems the concept of Reference Object is missing some important requirement (assumed implicitly): the resolved value MUST conform to the oneOf sibling of Reference Object.

Only few definitions clearly state requirements for the resolved value.
Path Item Object:

The referenced structure MUST be in the format of a Path Item Object.

Majority of definitions do not set requirements for resolved value or describe it as an option.
Responses Object:

A Reference Object can link to a response that the OpenAPI Object's components/responses section defines.

To me that is something to fix in the spec, but such fix could also be considered a breaking change, as some semantically broken schemas (failing to work with OpenAPI-aware tools) are still 100% compliant with current source-of-truth spec.

In my personal opinion allowing local references to point to any place in structure is unnecessary flexibility that contrubutes to overall complexity. I don't see why I would point a reference to some operation's response rather than extracting that shared response into component.

which will populate the first $ref pointing at an external resource with a resolved copy of that target, but subsequent instances of that $ref value with a new direct internal $ref to the first location.

I assume such tool leverages generic JSON Reference approach and is not aware of OpenAPI context when copying remote data, otherwise it would know what kind of reference is being processed and if there is a dedicated component for it.

The introduction of typed components was a great step forward from Swagger 2 definitions. Hopefully with one of next releases the use of components for local definitions (and maybe even remote definitions) will become mandatory. Such requirement can improve features orthogonality, overall tooling simplicity and validation reliability.

I don't mind if this PR is closed, but I would be more than happy if OpenAPI spec becomes stricter and less magical.

@MikeRalphson
Copy link
Member

MikeRalphson commented Dec 23, 2019

@OAI/tsc in its current form, motion to close this PR?

@vearutop if we don't already have an issue describing your concerns, could you create one from the description of this PR so it can be reconsidered when OAS 4 comes along?

@vearutop
Copy link
Contributor Author

@MikeRalphson I could not find existing issue on this regard, so I've created #2084.

@MikeRalphson
Copy link
Member

As this is a breaking change and we have #2084 to track the issue, closing this PR for now.

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.

4 participants