-
Notifications
You must be signed in to change notification settings - Fork 761
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
base: master
Are you sure you want to change the base?
Conversation
28eec21
to
2613f63
Compare
@char0n Would it be ok to remove to resolved externalValue and place the resolved value in the If it should be placed in Originally posted by @mathis-m in #1978 (comment) |
4374ca1
to
235f3f3
Compare
235f3f3
to
c158c2c
Compare
89b08cb
to
69efc1c
Compare
Yes,
We should check if I have a follow up question, do you have time to continue with this PR Draft? |
@char0n can try to finish this in the next week! |
69efc1c
to
346779d
Compare
@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.
|
@char0n have added the absolitfy for the url. No support for fragments until now. If needed please tell me. |
Yes it does, but swagger-client only supports OAS 3.0.3 at this point as a maximum OAS version. The resolution of
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
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)
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.
Not aware of any special ones |
@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 as you mentioned the base64 requirement is missing. I'll start testing this PR against the requirements of OAS 3.0.x specification. |
@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
Otherwise, are there other tests that you would have liked to see included in this PR? Thanks! |
Probably it is not good to base64 encode all other stuff. Imagine a external value that is a 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. |
Description
resolve external value to value
fixes or baseline for swagger-api/swagger-ui#5433
TODO
DONE
Motivation and Context
Fixes #1978
How Has This Been Tested?
currently not
Types of changes
package.json
)Checklist: