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

References to "definitions" #337

Closed
jsumners opened this issue Jan 26, 2021 · 16 comments · Fixed by #459
Closed

References to "definitions" #337

jsumners opened this issue Jan 26, 2021 · 16 comments · Fixed by #459
Assignees
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@jsumners
Copy link
Member

🐛 Bug Report

The "models" builder does not correctly reference embedded "definitions" style references.

To Reproduce

package.json

{
  "dependencies": {
    "fastify": "^3.11.0",
    "fastify-swagger": "^3.5.0"
  }
}

index.mjs

"use strict";

import Fastify from "fastify";
import * as fastifySwagger from "fastify-swagger";

const fastify = Fastify();
fastify.register(fastifySwagger, {
  routePrefix: "/_swagger",
  exposeRoute: true,
  swagger: {
    info: {
      title: "Test Swagger",
    },
  },
});

fastify
  .addSchema({
    $id: "urn:schema:base",
    definitions: {
      hello: { type: "string" },
    },
    type: "object",
    properties: {
      hello: { $ref: "#/definitions/hello" },
    },
  })
  .addSchema({
    $id: "urn:schema:ref",
    type: "object",
    properties: {
      hello: { $ref: "urn:schema:base#/definitions/hello" },
    },
  });

fastify.route({
  path: "/",
  method: "GET",
  schema: {
    response: {
      "2xx": fastify.getSchema("urn:schema:ref"),
    },
  },
  handler(req, res) {
    res.send({ hello: "world" });
  },
});

const response = await fastify.inject({ url: "/_swagger/json" });
console.log(JSON.stringify(response.json(), null, 2));

Expected behavior

The above reproduction will produce the following JSON:

{
  "swagger": "2.0",
  "info": {
    "title": "Test Swagger"
  },
  "definitions": {
    "def-0": {
      "definitions": {
        "hello": {
          "type": "string"
        }
      },
      "type": "object",
      "properties": {
        "hello": {
          "$ref": "#/definitions/hello"
        }
      }
    },
    "def-1": {
      "type": "object",
      "properties": {
        "hello": {
          "$ref": "urn:schema:base#/definitions/hello"
        }
      }
    }
  },
  "paths": {
    "/": {
      "get": {
        "responses": {
          "2xx": {
            "schema": {
              "$id": "todo.com",
              "type": "object",
              "properties": {
                "hello": {
                  "type": "string"
                }
              }
            },
            "description": "Default Response"
          }
        }
      }
    }
  }
}

If you were to start the server, instead of performing the .inject, and navigate to http://127.0.0.1:8000/_swagger/ and then click to expand def-0, an error will be presented like:

image

This is due to the definitions block in the JSON having a structure like definitions.def-0.definitions.hello. The UI expansion is instead expecting definitions.hello.

@mcollina
Copy link
Member

Good spot! Are you planning to send a PR to fix this?

@mcollina mcollina added bug Confirmed bug good first issue Good for newcomers labels Jan 26, 2021
@jsumners
Copy link
Member Author

Good spot! Are you planning to send a PR to fix this?

If I somehow find time and get annoyed enough by this, maybe. But this is a thing I'll probably overlook.

@mcollina
Copy link
Member

Well, it would be a good contribution for a newcomer!

@harveyconnor
Copy link

harveyconnor commented Jan 26, 2021

Yep getting the same issue here!

Resolver error at components.schemas.def-0.properties.resource.$ref
Could not resolve reference: undefined Route GET:/docs/Resource not found
Resolver error at components.schemas.def-0.properties.user.$ref
Could not resolve reference: undefined Route GET:/docs/User not found

@quiro91
Copy link

quiro91 commented Jan 28, 2021

Same here, I was looking to migrate from fastify-oas to fastify-swagger but got blocked by this

@mcollina
Copy link
Member

Maybe @climba03003 could take a look?

@climba03003
Copy link
Member

I may make a custom container for handle all the related schema, it can solve the encapsulation problem and better delivery for schema in openapi 3. I just need some times to dig inside fastify hooks for onRoute and onRegister.

@climba03003
Copy link
Member

climba03003 commented Jan 30, 2021

Some updates on this issue. I have been worked on the container structure, but it affect too much on the resolving the $ref keyword.
Some problem I am facing is that $ref seems to allow so many different format. For example, <file>.<json|yaml>#, Foo#, <links>#. I need to decide which one should be modified or left it there.

Some note on $ref when I research.

currently usage in fastify

I can handle it with fastify.getSchema
Foo#

inline definition

I can handle it by the check $ref.startsWith('#/definitions') and extract the definitions part to container
#/definitions/Foo

relative external definition

I may handle this by the check $ref.includes('#') && ( $ref.includes('.json') || $ref.includes('.yaml') ) and skip the resolving part.
foo.json#/Foo
bar.yaml#/Bar

absolute external definition

I may handle this by the check $ref.startsWith('http') and skip the resolving part.
http://example.com#/Foo
https://example.com#/Bar

I will fix the other existing bug first while investigating how to solve this.

@Eomm
Copy link
Member

Eomm commented Jan 31, 2021

@climba03003 regarding this, I review a PR here with some considerations about this merge (we must not forget that we are merging many little schemas to a BIG single one):
Eomm/json-schema-resolver#4 (review)

I must say that we should set the default behaviour as "user doesn't set strange schema's id" since the $id renaming works but it is annoying 😞

@jeremy-w
Copy link
Contributor

jeremy-w commented Apr 21, 2021

i managed to get references in schemata working with some more restrictive assumptions. doing so required some "bracketing" steps for my whole server configuration:

  • as soon as the server is created:
    • adding all local schemata with an added $id of their base filename to Fastify, so it can resolve the refs
    • registering static getter routes returning that edited schema for several variations on the filename (all combos of the two lists ["/", routePrefix] x [schemaFilename, sansJson]) to unblock Swagger UI and OpenAPI Generator reference evaluation
  • the tick after the onReady event is emitted:
    • postprocess the decorated server.swagger schema to fix up refs so they all point to something like #/definitions/SomeObjectName.

key to this working in this way is that destructively mutating the object vended from fastify.swagger has results that are directly visible in the generated Swagger UI and other endpoints registered by fastify-swagger.

the assumptions were:

  • all refs are to sibling files in a single directory
  • refs are written in those files as "$ref": "SiblingFile.json"
  • every file describes an object whose title is the basename minus extension, so SiblingFile is the title of the object in SiblingFile.json

fixup proved somewhat baroque as i navigated through the way these schemata wound up being edited during serializer/validator construction. i observed that:

  • the registered definitions all wound up named things like def-0 and def-12. references to these from handler schemata were updated by Fastify to use these names (e.g. #/definitions/def-12) rather than the original names.
  • defs in the registered definitions were not correspondingly updated, and so were broken - they stayed as originally written.
  • handler response body schemata that were refs were inlined, which would lead to duplicate class generation by OpenAPI Generator.

my fixup steps were basically:

  • use the fact every definition should have a title field to inject the definition in definitions as its title. leave the def-N names there for now so we can deref them to title during rewriting.
  • rewrite all the refs to drop .json and use the def in definitions, replace def-N with the corresponding title, and un-inline the ref by using title agreement as a trigger to nuke all fields and replace them with a single $ref field. (that last step was very intentionally skipped at paths info and any path under definitions to avoid a red herring in the first case and converting all defs to bare circular refs in the latter case.)
  • delete all the def-N definitions that we'd cloned now that we're done dereferencing everything.

this, together with setting the cwd for json2ts to the flat schemas directory, made it so all the tooling involved played nice, and now i can finally reuse specs directly.

@mcollina
Copy link
Member

mcollina commented May 3, 2021

@jeremy-w could we get a PR to fix this once and for all? Is there anything that could be added to this repo?

@jeremy-w
Copy link
Contributor

jeremy-w commented May 4, 2021

i didn't essay a PR because my solution is a very tight fit for a very restricted circumstance: i'm able to say "all refs are to local files in a single directory with no nesting" and "all objects must have a title" in my local project, and that's enough to let me fix everything up and undo the inlining of object definitions. other things i'm doing come from wanting to get openapi-generator-cli and json2ts to play nice with the generated schema (in the case of openapi-generator) and the original, on-disk schema files (in the case of json2ts).

i suspect fixing these next two points would mean fixes to fastify's schema handling itself, rather than something in fastify-swagger:

  • do not inline referenced definitions in response schemata, in order to avoid duplicating definitions
  • when renaming a ref to the def-N format, any references within the ref must also be renamed to avoid broken references

(re: the renaming, it would be even better if this renaming did not happen, but that would mean updating the module doing the ref resolution, which currently doesn't support controlling or disabling this renaming at all, before anything else could be done about it.)

but i don't know about any general solution: the issue that the ref resolution module, and any general solution to this problem, keep running aground on is the richness enabled by references, which can be local or remote, and can even dig into the structure of the referenced document (something i avoided having to support in my situation). this was called out in #337 (comment).

that said, if we did impose restrictions like the ones i adopted on refs used with fastify-swagger, then we could unlock that level of reuse for everyone. but it's definitely not "you can use whatever refs you want, and as long as they're valid, you're golden" - it's much more restrictive than that, and requires a specific arrangement and workflow.

@mcollina
Copy link
Member

mcollina commented May 4, 2021

cc @Eomm wdyt?

@MoJo2600
Copy link

MoJo2600 commented Jul 8, 2021

I'm not sure if I it is the same problem, but I get the following error when doing a $ref to a schema definition in a fastify querystring

UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'type' of undefined
    at resolveLocalRef (/home/runner/fastify-issue-3181/node_modules/fastify-swagger/lib/util/common.js:80:25)
    at plainJsonObjectToOpenapi3 (/home/runner/fastify-issue-3181/node_modules/fastify-swagger/lib/spec/openapi/utils.js:114:41)
    at resolveCommonParams (/home/runner/fastify-issue-3181/node_modules/fastify-swagger/lib/spec/openapi/utils.js:210:15)
    at prepareOpenapiMethod (/home/runner/fastify-issue-3181/node_modules/fastify-swagger/lib/spec/openapi/utils.js:290:29)
    at Object.swagger (/home/runner/fastify-issue-3181/node_modules/fastify-swagger/lib/spec/openapi/index.js:50:29)
    at /home/runner/fastify-issue-3181/index.js:68:11
    at manageErr (/home/runner/fastify-issue-3181/node_modules/fastify/fastify.js:475:11)
    at exit (/home/runner/fastify-issue-3181/node_modules/fastify/lib/hooks.js:90:5)
    at manageTimeout (/home/runner/fastify-issue-3181/node_modules/fastify/lib/hooks.js:107:11)
    at _encapsulateThreeParam (/home/runner/fastify-issue-3181/node_modules/avvio/boot.js:551:7)

I have a sample minmal repo here: https://replit.com/@mojo2600/fastify-issue-3181#index.js

@davidschmitt
Copy link

FYI, I have created a drop-in replacement for json-schema-resolver which solves many of the $ref related issues in external schemas for fastify-swagger.

I've published the code here: https://github.com/davidschmitt/fastify-schema-resolver.git

I haven't published the package on npmjs.com (I've never done that before), but if the fastify-swagger maintainers are interested in switching I can do so.

@Laurensdc
Copy link

Laurensdc commented Nov 30, 2023

Where is the $id: todo.com coming from? We're seeing that too.

Edit:

{ applicationUri: 'todo.com' },

Edit2:
We were using Typebox and didn't use Type.Ref() to reference the types. This resulted in the $id: todo.com to be added in the API spec.

Also providing an $id property where there shouldn't be one adds it in the API spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants