Remote $ref used like in the Swagger documentation does not work

[sirdiego]

As seen here: https://swagger.io/docs/specification/using-ref/ JSON Reference supports remote references with an element selector.

Using this test setup:

index.yaml

openapi: 3.0.3

info:
  version: '1.0'
  title: Test

paths:
  /:
    summary: Test

components:
  schemas:
    test:
      $ref: 'test.yaml#/test'

test.yaml

test:
  type: object
  properties:
    id:
      type: integer

I get the following error:

✨ swagger-to-ts 2.0
This library has been updated to 2.0 with improved generation. If you experience issues you can use the deprecated v1 with `npx @manifoldco/swagger-to-ts@1 …`
🤞 Loading spec from index.yaml…
SyntaxError: Invalid character. (9:23)
   7 | export interface components {
   8 |     schemas: {
>  9 |     "test": test.yaml#["test"];
     |                       ^
  10 | 
  11 |   }
  12 |     
    at e (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/parser-typescript.js:1:322)
    at Object.parse (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/parser-typescript.js:14:2893509)
    at Object.parse (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/index.js:13565:19)
    at coreFormat (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/index.js:14841:25)
    at format (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/index.js:15063:14)
    at formatWithCursor (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/index.js:15080:12)
    at /home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/index.js:54672:12
    at Object.format (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/node_modules/prettier/index.js:54692:12)
    at swaggerToTS (/home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/dist-node/index.js:394:19)
    at /home/tester/.npm/_npx/21480/lib/node_modules/@manifoldco/swagger-to-ts/bin/cli.js:49:18

[drwpow]

This should be fixed at least. But when generating schemas, what would you like to see the generated types become for this?

[sirdiego]

Hey @drwpow, thats a good question. I'd say define a new interface? 🤔

interface test {
id: int
}

[drwpow]

Yeah I think that’s heading in the right direction. It‘ll be tricky to resolve, for sure, but possible. I think the right fix would be a lot of work, because it‘d involve resolving those files (and downloading them, as we currently support remote specs). So I want to split apart a short-term fix with a longer one.

Short-term
I think just setting this to any for now would be a one-line fix to at least get this to generate. This would also add the logic we‘d need later to say “this is an external file.“ That external file will take work to resolve, but for now just set it to any so we can get part of the spec, as opposed to none. Not perfect, obviously, but better.

Future Scope
Defining an interface only for the imported file would be tricky, because OpenAPI allows schema names that aren‘t TS-safe. And when you rename things, there can be collisions (README has a note on this). Plus, if that file contains a $ref back to the original, that’d be tricky to resolve. Multiply if more files were added to the mix.

However, if you kept everything inline, that would work for TS, but what if test.yaml had a reference back to the original spec? All it would take would be one circular reference to make this pattern unusable (and I think that‘d be almost certain in most cases).

I‘ll spend a bit more time thinking on it, but here are 2 valid options I can think of off the top of my head:

1. Resolve it inline, but err on a circular reference. This would break down pretty quickly, as I think circular references are highly-encouraged. I think this would nix the main advantage of remote references like your issue points out.
2. Invent a new interface just for external files. I think your suggestion would work with a minor tweak:

interface components {
  { // …
    "$ref": ExternalFiles["test.yaml"]["foo"]
  }
}
interface ExternalFiles {
  "test.yaml": {
      foo: // …
    }
  }
}

This would allow circular references back-and-forth, while being backwards-compatible with the current API.

I think the only downside would be if you were trying to generate types for both specs, you‘d have duplication. But I think that‘d be expected, right? Because both files reference each other.

WDYT about implementation?

[sirdiego]

Hi @drwpow, thanks for your reply. I like the short term fix. This way the configuration would not throw.

On the interface with the refs I'm not so sure. This way when we use this inside our code we would have to know the filename inside the OpenAPI project to use the type inside the programs code. I don't really know how the OpenAPI spec defines what should happen with circular references. From what I understand the $ref-block is 'replaced' by the corrosponding code - that would indeed break down pretty fast.

It would be awesome to just be able to use that interface inside the code without having to use the components.$ref path.

Maybe namespaces could be handy here?

[drwpow]

Oh you’re right. I made a mistake in my example. I should have illustrated that $ref goes away. Agreed that you should be able to use the code without $ref. I don’t think you should have to know the filename, either. Just internally, we do need a way to keep collisions apart, somehow, so we’d need some separation. But that may be more of an implementation detail.

I think the rest of it may become more clear as we try and prototype this out. One unknown for me is how much of the imported file you can access; I’d probably lean toward “only what the original spec references specifically.”

Anyways, I’ll get a quick fix out so the spec can generate, and will make a followup PR later for the longer-term fix (may take a while though).

[drwpow]

v2.3.4 released with the quick workaround. Hopefully that unblocks you for now!

[nponeccop]

FYI there are libraries out there implementing the RFC6901 an the $ref draft. E.g.

https://www.npmjs.com/package/@apidevtools/json-schema-ref-parser

JSON Schema $Ref Parser is a full JSON Reference and JSON Pointer implementation that crawls even the most complex JSON Schemas and gives you simple, straightforward JavaScript objects.

It does resolve all references including external ones.

[drwpow]

@nponeccop that‘s a great find, thank you! I was not aware of namespaced schemas like this. If this is a good solution, then we could definitely add support for remote schemas in a future release.

[drwpow]

Since there are now quite a few requests for this, I’d love to add it. But there’s one problem: I think this would mean that the Node.js API for this library, which is currently synchronous, would have to be async. Because if it encounters remote schemas, it now has to fetch them (I know that if all $refs are local we could keep it sync, but I’d rather just allow for remote $refs and if it’s sometimes async, just make it always execute async).

This may cause a little disruption to some people using the Node.js API, but I think moving to async overall would be a win. CLI wouldn’t change.

What are peoples’ thoughts? 👍🏻 or 👎🏻 ?

[josh-feldman]

IMO it would be fine to make it async-only and publish a breaking change. I will note that the most common use case for this package is in some sort of build step, which doesn't typically involve asynchronous operations. That being said I can't imagine it creating too big a lift for someone to update a few build scripts to async to support this.

If we really need to keep backwards compatibility, we could alternatively provide two functions and log warn if a remote ref is encountered when using the sync version.

[nponeccop]

this would mean that the Node.js API for this library, which is currently synchronous, would have to be async

My opinion is that we should use the same approach to async schemas that ajv has:

ajv.compile(schema: object): (data: any) => boolean | Promise < any >

So we do sync validation whenever we can. But if it requires extra work, we could KISS by always doing async. Also, readFileSync is a hack and with modern SSD drives is a performance problem.

As for breaking changes, we can do it two ways:

• break it however we want, and increment the major version
• add ..Async versions of the API

[drwpow]

Just wanted to give an update on this: I’m making progress on this feature and it’ll likely go out in the next minor release!

Thanks to @nponeccop for the suggestion on json-schema-ref-parser. That was a big inspiration to unblocking this. I’ll have a PR up later this week that can act as a RFC for those interested.

[drwpow]

PR is up! Please take a look specifically at this test snapshot to get a feel for the newly-generated code: https://github.com/drwpow/openapi-typescript/pull/602/files#diff-884150d72ef4b660d8e83b0fcef66aeea1c8c1da9eddfebed53176ac43d4ebe7

[drwpow]

Released this as openapi-typescript@next. Going to do a little testing before releasing this as latest.

Again, event though it’s a 4.0 breaking version, that will only be for Node.js API users which must now await on a promise. CLI users should see no disruption and this should be backwards-compatible with generated schemas.