$ref support - OAS 2.0 compliant #239

Eomm · 2020-05-06T20:23:27Z

Opening this PR as POC:
when the user uses $ref, swagger-ui will do an HTTP GET to retrieve the external JSON.
So, to let this works, we need to:

expose many routes as many fastify.addSchema() the user does
the user should use $id: hello because if he uses $id: http://example.com.. the GET will go to http://example.com and it won't work!!
write a good tutorial 😄

I will rebase it to #238
This plugin could benefit from the changes too: SkeLLLa/fastify-oas#30

Writing this feature I will check the OAS 2.0 feature supported to add those who are missing like #161

My personal achievements would be to fix all the "invalid schema error" that doesn't let me generate a Postman collection form the generated swagger

mcollina · 2020-05-07T09:37:58Z

I think we should really try to resolve all the internal references if possible. I think it provides a better user experience, mainly because somebody can download all the files and store them in version control if they need to auto-generate a client.

Would it be opposed to that @Eomm? I think it's just a matter to resolve all the $ref

Eomm · 2020-05-07T20:10:54Z

I would resolve URI like $id: 'myawesome:schema' and not URL $id: 'http://my-server/schema'

One single file output seems mandatory to me also to let users generate docs or other useful stuff

mcollina · 2020-05-08T08:53:07Z

I would resolve URI like $id: 'myawesome:schema' and not URL $id: 'http://my-server/schema'

Agreed.

One single file output seems mandatory to me also to let users generate docs or other useful stuff

Agreed.

SkeLLLa · 2020-05-09T18:38:57Z

routes.js

+  //     throw err
+  //   }
+  // })
+  const allSchemas = fastify.getSchemas()


@Eomm, do you know if in v3 this method was reworked, so it will return all schemes, even those what were registered in child scopes with fastify.register?
Cause in fastify v2 I had to go through all child scopes in order to get all of them like https://github.com/SkeLLLa/fastify-oas/blob/master/lib/openapi/index.js#L5

nope, it is encapsulated and tested here:
https://github.com/fastify/fastify/blob/master/test/schema-feature.test.js#L505

I agree that we could think a nicer solution 👍

I think looping through children is quiet fine, unless you have schemes with same names. But in fastify-oas module there were no such issues, so it can be used.

In think the onRegister hook could let us to avoid to use the symbol, I will give it a try

One more idea - is to add addSchema hook to Fastify. That will allow plugins to get schemes when they are registered. And, for example, instead of resolving them we could put schema to swagger definitions when addSchema is called and use $ref inside swagger route to that definition.

dynamic.js

Eomm · 2020-05-18T20:54:04Z

This pr is harder than I thought... now I understand why json-schema would like to standardize with OAS

mcollina · 2020-05-19T09:33:34Z

This pr is harder than I thought... now I understand why json-schema would like to standardize with OAS

Why?

Eomm · 2020-05-19T12:08:16Z

I wanted to replace $ref to point to local definitions, as suggested by the RFC.

But the swagger config support $ref to definitions only for the body;

headers, params and query parameter do not support $ref out of the box so I must adopt another approach

Eomm · 2020-05-20T22:40:39Z

Update: missing more tests, than it should be ready to review

Eomm · 2020-05-21T21:44:19Z

I think this PR is ready for feedback

I would like a check by @SkeLLLa too if you could be so kind to give me your though

mcollina

good work!!

dynamic.js

mcollina · 2020-05-21T21:54:29Z

dynamic.js

+    instance.addHook('onReady', (done) => {
+      const allSchemas = instance.getSchemas()
+      for (const schemaId of Object.keys(allSchemas)) {
+        if (!sharedSchemasMap.has(schemaId)) {


Can we detect if the id is being overridden by something else?

Maybe here we can add instance prefix to schema name if it exists? In some cases I guess it could solve some schema conflicts.

The problem here is that onRegister is receiving for all the register so in this case:

register root - addSchema 1

child 1 registered - addSchema 2

subchild 2 registered - addSchema 3

For subchild 2 i will read schema 1, 2, 3
For child 1 schema 1 and 2
For root only schema 1

So it will be quite normal to have duplicate and I think it is ok since the addSchema already has a check to verify duplicate $id

I mean slightly different case.

root - schema-1

child1 (prefix '/news') - item

child2 (prefix '/comments') - item

So if item schemas are registered in isolated scopes they will know now nothing about each other. However when you gather all them in shared schema map they will have the same id.

Could you leave a TODO comment in here? Or open an issue? I think we might want/be able to have a way to get all schemas defined at the current level so you do not get them all here.

I agree and I will describe the case 👍

routes.js

test/json-schema.test.js

SkeLLLa

@Eomm
Besides that thing with potential schema conflicts and Matteo's comments, LGTM.

SkeLLLa · 2020-05-22T07:18:16Z

dynamic.js

+    instance.addHook('onReady', (done) => {
+      const allSchemas = instance.getSchemas()
+      for (const schemaId of Object.keys(allSchemas)) {
+        if (!sharedSchemasMap.has(schemaId)) {


Maybe here we can add instance prefix to schema name if it exists? In some cases I guess it could solve some schema conflicts.

mcollina

lgtm

SkeLLLa

LGTM. I guess it will support most of common cases, for others it's possible to modify behaviour without any breaking changes needed.

Eomm · 2020-05-24T09:31:42Z

I must add some other commit.. I forgot the response 😱
I'm working on it now

mcollina

lgtm

ahgentil · 2020-05-28T20:29:13Z

I have my schemas in multiple files.
One of them has common stuff like:

  $id: 'http://example.com/',
  definitions: {
    uuid: {
      type: 'string',
      pattern: '^[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}$'
    }
...

Then inside other plugins' schemas I reference that:

id: { $ref: 'http://example.com/#/definitions/uuid' },

So, I just noticed that it blows up here:

fastify-swagger/dynamic.js

Line 381 in 32d4f42

if (jsonSchema.type && jsonSchema.properties) {

because, as the function name states localRefResolve(), this ends up returning undefined:

fastify-swagger/dynamic.js

Lines 392 to 396 in 32d4f42

    
           if (jsonSchema.$ref) { 
        
             // $ref is in the format: #/definitions/<resolved definition>/<optional fragment> 
        
             const localReference = jsonSchema.$ref.split('/')[2] 
        
             return localRefResolve(externalSchemas[localReference], externalSchemas) 
        
           }

Should this $ref resolver be able to resolve any $ref?
The uuid definition is in the externalSchemas param.

Eomm · 2020-05-28T20:40:55Z

Could you open an issue with a complete example?
I get the point but it is time spending recreate it

ahgentil · 2020-05-28T21:34:17Z

voilá #252

Eomm added 2 commits May 6, 2020 19:35

update dept

c36683e

add fast poc $ref

a1ebe92

SkeLLLa reviewed May 9, 2020

View reviewed changes

Eomm mentioned this pull request May 16, 2020

Update fastify to v3 #238

Merged

4 tasks

Eomm added 3 commits May 16, 2020 17:33

Merge branch 'master' into fastify-v3

72d2250

wip to resolving

adf995e

resolving body $ref

15a83cb

mcollina reviewed May 17, 2020

View reviewed changes

dynamic.js Outdated Show resolved Hide resolved

shared schema across http parts

2aefca1

manage query path params and header

cba6c78

Eomm added 4 commits May 21, 2020 23:05

add test

2bfeb51

fix use onReady hook

1e16a49

add swagger file support

76d0eee

fix tests

4f2a8da

Eomm marked this pull request as ready for review May 21, 2020 21:44

Merge branch 'master' into fastify-v3

4440425

mcollina reviewed May 21, 2020

View reviewed changes

SkeLLLa reviewed May 22, 2020

View reviewed changes

Eomm added 3 commits May 22, 2020 22:20

update feedback

f1bb6a6

update dep from beta

945d99c

fix removed unused code

61095a6

mcollina approved these changes May 23, 2020

View reviewed changes

SkeLLLa approved these changes May 23, 2020

View reviewed changes

add response $ref support

3f27e12

mcollina approved these changes May 24, 2020

View reviewed changes

Eomm merged commit 1bd5447 into fastify:master May 24, 2020

Eomm deleted the fastify-v3 branch May 24, 2020 18:50

Eomm mentioned this pull request May 24, 2020

wip: show file upload on swagger #161

Closed

Eomm mentioned this pull request Jun 1, 2020

Migration guide update fastify/fastify#2260

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

$ref support - OAS 2.0 compliant #239

$ref support - OAS 2.0 compliant #239

Eomm commented May 6, 2020

mcollina commented May 7, 2020

Eomm commented May 7, 2020

mcollina commented May 8, 2020

SkeLLLa May 9, 2020

Eomm May 9, 2020

SkeLLLa May 9, 2020

Eomm May 9, 2020

SkeLLLa May 10, 2020 •

edited

Loading

Eomm commented May 18, 2020

mcollina commented May 19, 2020

Eomm commented May 19, 2020

Eomm commented May 20, 2020

Eomm commented May 21, 2020

mcollina left a comment

mcollina May 21, 2020

SkeLLLa May 22, 2020

Eomm May 22, 2020

SkeLLLa May 23, 2020

mcollina May 23, 2020

Eomm May 24, 2020

SkeLLLa left a comment •

edited

Loading

SkeLLLa May 22, 2020

mcollina left a comment

SkeLLLa left a comment

Eomm commented May 24, 2020

mcollina left a comment

ahgentil commented May 28, 2020

Eomm commented May 28, 2020

ahgentil commented May 28, 2020

$ref support - OAS 2.0 compliant #239

$ref support - OAS 2.0 compliant #239

Conversation

Eomm commented May 6, 2020

mcollina commented May 7, 2020

Eomm commented May 7, 2020

mcollina commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkeLLLa May 10, 2020 • edited Loading

Choose a reason for hiding this comment

Eomm commented May 18, 2020

mcollina commented May 19, 2020

Eomm commented May 19, 2020

Eomm commented May 20, 2020

Eomm commented May 21, 2020

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkeLLLa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

SkeLLLa left a comment

Choose a reason for hiding this comment

Eomm commented May 24, 2020

mcollina left a comment

Choose a reason for hiding this comment

ahgentil commented May 28, 2020

Eomm commented May 28, 2020

ahgentil commented May 28, 2020

SkeLLLa May 10, 2020 •

edited

Loading

SkeLLLa left a comment •

edited

Loading