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

feat: resolve external value #2013

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mathis-m
Copy link

@mathis-m mathis-m commented Apr 5, 2021

Description

resolve external value to value

fixes or baseline for swagger-api/swagger-ui#5433

TODO

2.) externalValue can be relative or absolute URL

externalValue can be either relative or absolute URL. If it's relative it's resolved according to these rules.

DONE

1.) The value field and externalValue field are mutually exclusive.

We don't resolve externalValue if value was defined.

3.) externalValue eventual value is not interpreted and is always represented as a string

After we resolve externalValue and fetch it, we don't try to interpret it in any way. By Interpreting I mean trying to parse JSON or YAML strings into JavaScript objects. We always transclude whatever is under externalValue URL as a string

Motivation and Context

Fixes #1978

How Has This Been Tested?

currently not

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mathis-m mathis-m marked this pull request as draft April 5, 2021 01:09
@mathis-m mathis-m force-pushed the ft/external_value branch 6 times, most recently from 28eec21 to 2613f63 Compare April 5, 2021 01:35
@mathis-m
Copy link
Author

mathis-m commented Apr 5, 2021

@char0n Would it be ok to remove to resolved externalValue and place the resolved value in the .value path instead.

If it should be placed in .externalValue how could this be accomplished. Plugin seems to be called while externalValue key exists, so my questions is probably how to mark a fullPath to exernalValue as resolved to not call the plugin with it again and again.

Originally posted by @mathis-m in #1978 (comment)

@mathis-m mathis-m force-pushed the ft/external_value branch 2 times, most recently from 4374ca1 to 235f3f3 Compare April 5, 2021 14:38
@mathis-m mathis-m closed this Apr 5, 2021
@mathis-m mathis-m reopened this Apr 5, 2021
@mathis-m mathis-m force-pushed the ft/external_value branch 5 times, most recently from 89b08cb to 69efc1c Compare April 5, 2021 16:12
@char0n
Copy link
Member

char0n commented Sep 10, 2021

@char0n Would it be ok to remove to resolved externalValue and place the resolved value in the .value path instead.

Yes, externalValue should stay unmodified, and the resolved thing should be put in value field.

If it should be placed in .externalValue how could this be accomplished. Plugin seems to be called while externalValue key exists, so my questions is probably how to mark a fullPath to exernalValue as resolved to not call the plugin with it again and again.

We should check if value field is set and stop the processing.

I have a follow up question, do you have time to continue with this PR Draft?

@mathis-m
Copy link
Author

@char0n can try to finish this in the next week!

@mathis-m
Copy link
Author

@char0n currently I am reading the OAS Spec 3.1.0, it is stating that externalValue should follow the Relative References Section. This section states that it should also resolve fragments.

  1. Is this something I need to take care of?
  2. What are the cases that need to be handled?:
  • full url
  • url relative to base docuement
  • fragment
  1. Are there any util functions that can be used for this?

@mathis-m
Copy link
Author

mathis-m commented Sep 23, 2021

@char0n have added the absolitfy for the url. No support for fragments until now. If needed please tell me.
Needed to increase entrypoint size. Maybe we could refactor the absolitfy from refs plugin to helpers to save some bytes.

@mathis-m mathis-m marked this pull request as ready for review September 23, 2021 19:40
@char0n char0n self-requested a review September 24, 2021 08:11
@char0n char0n self-assigned this Sep 27, 2021
@char0n
Copy link
Member

char0n commented Sep 29, 2021

@char0n currently I am reading the OAS Spec 3.1.0, it is stating that [externalValue should follow the Relative References Section]

Yes it does, but swagger-client only supports OAS 3.0.3 at this point as a maximum OAS version. The resolution of externalValue happens as specified in this comment and was verified with the spec author.

This section states that it [should also resolve fragments]

As mentioned above, this is part of OAS 3.1.0 spec. But even if we were to implement OAS 3.1.0 behavior, the fragment in externalValue URL IMHO doesn't really make sense as the fragment is ignored by any requesting mechanism and the expected resolved value is of arbitrary type.

  1. Is this something I need to take care of?

If this question is specific to your questions of OAS 3.1, then I'd say no, we don't care about those thing as they just don't apply here. We only care about #1978 (comment)

  1. What are the cases that need to be handled?:
  • full url
  • url relative to base docuement
  • fragment

We need to handle full url and relative url. We don't care about fragments. We resolve relative URL agains the Server.url not agains the baseURI.

  1. Are there any util functions that can be used for this?

Not aware of any special ones

@mathis-m
Copy link
Author

mathis-m commented Oct 1, 2021

@char0n besides the review comment I made are there any changes need. I think the PR currently fulfills the stated behavior from your last comment.

@mathis-m
Copy link
Author

mathis-m commented Oct 4, 2021

> If the file extension if different then .json, '.yamlor.ymlwe shouldbase64 encodethe string before we transclude the resolved value tovalue` field.

I think this is still missing,

@char0n
Copy link
Member

char0n commented Oct 8, 2021

@mathis-m as you mentioned the base64 requirement is missing.

I'll start testing this PR against the requirements of OAS 3.0.x specification.

@char0n char0n removed their request for review December 14, 2021 09:20
@char0n char0n removed their assignment Dec 14, 2021
@tim-lai
Copy link
Contributor

tim-lai commented Sep 30, 2022

@char0n I updated this PR that fixes merge conflicts, test and lint errors on a new branch mathis-m-ft/external_value. (mostly b/c I couldn't push to this PR directly). It looks like all three primary case are now covered, including handling of absolute vs relative url. The new tests provided now all pass.

However, can you clarify why this comment needs to be included in this PR as well, given that it appears to not otherwise be present in the refs.js case either? Seems to me it should be a different feature/issue?

If the file extension if different then .json, '.yamlor.ymlwe shouldbase64 encodethe string before we transclude the resolved value tovalue` field.

Otherwise, are there other tests that you would have liked to see included in this PR?

Thanks!

@mathis-m
Copy link
Author

mathis-m commented Oct 7, 2022

Probably it is not good to base64 encode all other stuff. Imagine a external value that is a .txt, .xml or any other readable format.
base64 does not make sense for all types of external values, maybe it only does for e.g. images.

As reference take the spec defined in #1978

The min value is fetched from a txt file. Which is totally fine to do, then I expect that the number value in that file is put into the min value of the spec.

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.

feat: externalValue resolver
3 participants