Added support for referencing remote schemas

[CodeLenny]

Hi @auscaster,

I've been exploring support for referencing remote files. I've managed to hack support in, but it's very ugly.

Given that you're more familiar with the code base, I wondered if you'd have an opinion of the cleanest way to get the data needed to read remote files.

It looks like the only things that common.resolveSchemaReference needs is a path to the current file (to resolve relative references) and options to pass along to /lib/preprocessor.

• Clean relative references
• Clean passing options for preprocessor.

I haven't tested deep file references, but OpenAPI's petstore-seperate is working for me (although I did flatten the file paths, and should test on the original).

• Build doesn't error out when a remote file is referenced
• Response examples from remote files are included

---

Task list has been re-organized since the first message in this PR. The below list is the current state of the PR. Please note that many changes were directly implemented, and never added to the list.

• Tests

  • Test complex paths
  • Test deep file references
  • Test files requested over HTTP
  • Test "contexts"
  • Add failure-case tests (replaceReference, replaceRefs)
  • Document testing in README
  • Add remote reference inside cheesestore.json

• Remote Schema Evaluation

  • Remote URLs could be cached to provide better performance (if not desired, remove TODO comment on fetchReference)
  • Relative URLs don't work with Remote URLs
  • Local paths in external files aren't handled (see replaceRefs)
  • "reference contexts" (utilities that recognize "/responses/[code]/schema" as something to insert into "/definitions/[...]"
    • Add additional contexts (2nd PR)
    • Make contexts optional (2nd PR)

• Tags

  • Handle tags as needed

• UI

  • Tweak names to remove paths? (in definitions, Pet.yml -> Pet, defs.yml#/things/Pet -> Pet) (2nd PR)
  • Add "Imported From" (2nd PR)
  • Add tests?
  • Schema definitions aren't copied into the main page

Random issues noted that should be checked at the end:

• Parameter definitions aren't working
• Path ordering is done before paths in external references have been added

[auscaster]

Hey there :)

It will be great to get this feature into spectacle, thanks a lot for your work!

As you say there are a few things to consider before we merge this into the core, but you seem to have a solid handle on them.

I wonder, however, if it wouldn't be cleaner to to load remote references on the first call to the processor, so we can keep that logic out of the view helpers. If fact thinking about it this is the only way we will be able to pull external schema definitions into the root document cleanly and without issue.

[CodeLenny]

@auscaster Glad to have your support!

Moving this into the processor sounds much better. Would you be able to point me to where the reference loading would fit in nicely? It looks like the existing code might not iterate over keys deep enough to find all references, but I haven't explored the preprocessor in depth yet.

[auscaster]

True there is no code that does that in the preprocessor.

The easiest way would be to use recursion, something like this:

// Feed the JSON object into a function like this
function eachRecursive(obj) {
    for (var k in obj) {
        if (typeof obj[k] === "object" && obj[k] !== null)
            eachRecursive(obj[k]); // check next level
        else
            // chech `obj[k]` for external references that need to be resolved and do work, or continue... 
    }
}

[CodeLenny]

@auscaster Glad I read it correctly! Should I add the recursive loop to load references before looping over the HTTP methods or would it fit somewhere else?

[auscaster]

At the end of the preprocessor would be best.

…

On 3 March 2017 at 20:19, Ryan Leonard ***@***.***> wrote: @auscaster <https://github.com/auscaster> Glad I read it correctly! Should I add the recursive loop to load references before looping over the methods <https://github.com/CodeLenny/spectacle/blob/e5e46227360f55e6a7845fefc21ed0484edf9118/app/lib/preprocessor.js#L25> or would it fit somewhere else? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGKDFl50BzaOTQSwsckb8qOnLpckavrks5riGfCgaJpZM4MRu62> .

[CodeLenny]

@auscaster Will do.

Another question: Should imported definitions/parameters/responses be added to the global objects?

For instance, should the following files:

# petshop-seperate.yaml
swagger: "2.0"
paths:
  /pets:
    get:
      description: Get all the pets.
      responses:
        "200":
          schema:
              type: array
              items:
                $ref: 'Pet.yaml'

# Pet.yaml
type: object
required:
  - id
  - name
properties:
  id:
    type: integer
    format: int64

Be equivalent to:

# petshop-unitifed.yaml
swagger: "2.0"
paths:
  /pets:
    get:
      description: Get all the pets.
      responses:
        "200":
          schema:
              type: array
              items:
                $ref: '#/definitions/Pet'
definitions:
  Pet:
      type: object
      required:
        - id
        - name
      properties:
        id:
          type: integer
          format: int64

Meaning the Pet object definition would be included in the docs with all other definitions?

I could see deep definitions providing an additional hurdle, and either way, the URL re-writer would have to be re-implemented (it currently does "#/definition/" -> "#definition-")

[auscaster]

Hey there, nice to see you made some progress! Yes I think we need to merge the referenced objects into the root spec, and make the definitions available or the resulting document will have broken links.

[CodeLenny]

@auscaster Glad to hear you agree. Shall I just insert definitions into the original YAML/JSON document, or do you want a fancier method?

When I'm merging, I'm guessing I can use some relative path to the file to keep references seperate:

definitions:
  # From the primary document
  Pet: ...
  # From an external document
  "../defs/Pet": ...

Or do you have a cleaner way to put the name?

Thanks again!

[auscaster]

Hmmmm .... I think be can just merge them into the main document, but maybe we add a custom internal _external attribute (underscore to flag internal) and assign it with the external file name or URL. ie.

definitions:
  # From the primary document
  Pet: ...
  # From an external document
  Pet:
    _external: <external filename or URL>

Then when we generate the documentation we can render the _external a field below the description with some text like "Imported from "

What do you think?

[CodeLenny]

@auscaster And I guess if the reference isn't to a specific object in a file, we should use the filename as the entry in the definitions section?

If someone linked to defs.yaml#/jobs/Programmer, we can easily create:

definitions:
  Programmer:
    _external: "defs.yaml#/jobs/Programmer"

And if someone linked to jobs/Architect.yaml, we use:

definitions:
  Architect:
    _external: "jobs/Architect.yaml"

An alternative is to provide the entire path in the key. This would fix problems if two documents used the same definition name, like "Error".

definitions:
  "../users/defs/Error": {}
  "../items/defs/Error": {}

Which would also give us unique paths in URLs: index.html#../items/defs/Error. When creating the human readable presentation, it could simplify the section title down to just "Error", and provide the full field underneath, with "Imported from".

I've found other documentation systems like codo only create URLs based on class name, when I might want to use the same class name in different files, which has caused some problems. If there might be similar use cases (like generic names, Error, Options, Data), it would be nice to have a plan taking them into account.

[auscaster]

I like your approach using keys, and you are right it will prevent collisions and make linking easier :)

…

On 10 March 2017 at 17:53, Ryan Leonard ***@***.***> wrote: @auscaster <https://github.com/auscaster> And I guess if the reference isn't to a specific object in a file, we should use the filename as the entry in the definitions section? If someone linked to defs.yaml#/jobs/Programmer, we can easily create: definitions: Programmer: _external: "defs.yaml#/jobs/Programmer" And if someone linked to jobs/Architect.yaml, we use: definitions: Architect: _external: "jobs/Architect.yaml" An alternative is to provide the entire path in the key. This would fix problems if two documents used the same definition name, like "Error". definitions: "../users/defs/Error": {} "../items/defs/Error": {} Which would also give us unique paths in URLs: index.html#../items/defs/Error. When creating the human readable presentation, it could simplify the section title down to just "Error", and provide the full field underneath, with "Imported from". I've found other documentation systems like codo <https://github.com/coffeedoc/codo> only create URLs based on class name, when I might want to use the same class name in different files, which has caused some problems. If there might be similar use cases (like generic names, Error, Options, Data), it would be nice to have a plan taking them into account. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGKDA0X7nwouEWC88G2a8_6y8xITlnEks5rkYARgaJpZM4MRu62> .

[CodeLenny]

@auscaster Great!

I'm breaking the program down into some simpler functions, and would like to write some tests, both to ensure the program functions as desired, but also to formalize the functionality that I'm adding.

Could I include some tests in the repository? I generally write tests with Mocha and Chai, but I can use another system if desired.

Thanks!

[auscaster]

That would be awesome .... please go ahead and add some proper testing with Mocha and Chai! The current system really needs it :+1

…

On 12 March 2017 at 23:10, Ryan Leonard ***@***.***> wrote: @auscaster <https://github.com/auscaster> Great! I'm breaking the program down into some simpler functions, and would like to write some tests, both to ensure the program functions as desired, but also to formalize the functionality that I'm adding. Could I include some tests in the repository? I generally write tests with Mocha and Chai, but I can use another system if desired. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGKDFmEBfq80QI5pvMDqk1eJ1-DpkElks5rlG0-gaJpZM4MRu62> .

[CodeLenny]

@auscaster Cool, I've already started. I assume I can export some of the new functions I'm adding to preprocessor, and change npm test to run Mocha? Then it can be integrated with Travis CI or a similar test runner.

[auscaster]

Nice :) Have you used circleci before? I use it for all my projects and its great.

…

On 13 March 2017 at 01:17, Ryan Leonard ***@***.***> wrote: @auscaster <https://github.com/auscaster> Cool, I've already started. I assume I can export some of the new functions I'm adding to preprocessor, and change npm test to run Mocha? Then it can be integrated with Travis CI or a similar test runner. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGKDG9IAAtlADE6m1S5dlC8O1s0yxNrks5rlIsmgaJpZM4MRu62> .

[CodeLenny]

@auscaster I haven't, so I'd probably let you set it up.

[CodeLenny]

@auscaster I've added a first testing file. Do you want to see if my testing style (Mocha + Chai 'should' syntax) matches yours enough? It would be relatively simple to change it at this stage, before I write a lot more tests.

[auscaster]

Your test syntax is great, and even better, tests are passing!

Please go ahead with what you are doing :)

[CodeLenny]

Glad to hear it!

I've been thinking more about how to structure the loops in the preprocessor.

According to the OpenAPI spec, references can only be used in a few specific places. For the most part, we want to simply replace the reference line with the contents of the remote file. However, there are a few cases where we want to do additional actions (such as inserting the remote contents into the definitions section, and replacing the external reference with a local reference).

I can see two approaches:

1. We search out the few places the spec allows references, and only evaluate those. For instance, we assume that there's a paths object containing objects for each path. We only go into paths, and ignore other sections and fields like info.

Pros

• Almost certainly more efficient
• Provides a more spec-driven behavior, with fewer side-effects

Cons

• Must write custom code for each $ref "scenario" (and might miss some)
• Must be kept revised when spec changes

2. We do the previously proposed global search, walking the entire tree to find any usage of $ref that links to an external source, and replace the object with the remote contents. If we recognize the current path as one of the special cases (like definitions to move elsewhere in the local document) we can apply special processing.

Pros

• Allows flexibility in use cases (some users might want to have info in a different document)

Cons

• "Recognizing" the current path isn't as pretty

---

@auscaster I'm not sure which is better. I'm leaning towards option 2, but I'd be interested in hearing what you think.

[auscaster]

@CodeLenny if we can probably reduce code complexity at a negligible performance hit and get the desired result then let's do it, #2 seems like the simpler approach

[auscaster]

By the way I just wanted to say thanks for the effort you're putting into spectacle 👍

I'll create a contributors section on the README soon and would very much like to add you.

[CodeLenny]

@auscaster I've started approach 2.

I'm happy to help, we're using the software at my office, and wanted to add this feature for our own use.

I'm trying to find where you parse YAML files. It appears that loadData in index.js just calls require() on whatever spec file is given:

return require(path.resolve(opts.appDir + '/lib/preprocessor'))(
                                    options, require(specPath));

I didn't think that require could deal with YAML files, but I might be mistaken. Is there something I'm missing?

Thanks!

[CodeLenny]

@auscaster Any ideas about how YAML files are parsed? I've been studying all the code, and can't figure it out.

Thanks!

[CodeLenny]

@auscaster I already updated the PR Summary to include those items as tasks:

• Tweak names to remove paths? (in definitions, Pet.yml -> Pet, defs.yml#/things/Pet -> Pet) (2nd PR?)
• Add "Imported From" (2nd PR?)

As I haven't touched the UI side of the code yet in this PR, I'd be interested in seeing if things work "well enough" as is for this PR, and leave the full UI tweaks for later.

However, it would be nice to have a few tests to ensure that at least the current functionality (with .yaml) continues working.

[auscaster]

👍

[CodeLenny]

@auscaster I've polished a few more parts of the code.

1. I've updated the README with a brief explanation about testing methodology.
   Please let me know if you have any suggestions for the text.

2. Paths are being inserted out of order. I wasn't able to figure out what part of the code reorders the paths.
   Would you be able to easily point out where ordering is taken into account, or should I dig deeper?

3. I'm a little uncomfortable about merging this without adding any tests confirming that the outputted UI is acceptable, but I'm unfamiliar with your UI code, and it seems a lot to dig into just to add a little testing.
   Would you be all right if we moved forward with the PR without touching the UI, if the output looks acceptable?

4. Have I missed anything in my PR Summary that should be added to this PR?

I've still got a few last changes to finish, but it looks like this PR is getting close to being done.

Thanks as always!

[auscaster]

Hello

Everything looks to be in order - except paths ;) Regarding that paths are currently ordered by Tags, which allows for the tab based categorization in the sidebar template here. It calls the eachSorted method which may need to be refactored to get paths displaying in the desired order. Is that something you need?

As long as we can get the output looking good I am happy. What I might do is add some remote swagger items to cheesestore.json just so at least the UI code gets touched on each call to npm test, and the remote swagger markup will be visible on the live demo.

[CodeLenny]

@auscaster Ah yes, I completely ignored tags. I'll take a look at implementing that when I add paths from external files.

I'll look at adding remote items to cheesestore.json.

[CodeLenny]

@auscaster Any chance Circle CI could be linked with GitHub so PRs are tested, and the result is displayed in the GitHub UI? Would make me more comfortable :)

[auscaster]

Hi there, sorry for the slow response. Hardly surprising though since my company commitments have really ramped up lately!

I've added CircleCI to pull requests .... hopefully it will work for existing PRs?

Hows it all coming together on your end?

[CodeLenny]

Hey @auscaster, it's been busy here as well, but I should have some free time now to work on the PR again.

It looks like CircleCI didn't run tests on this PR. It might be complaining that this PR is from my own branch - I know Travis detects this and blocks external CIs from seeing private variables, as I might adjust the CI config to echo $SECRET_PASSWORD or something. Would it be possible to look into this more?

Looks like I mainly just need to deal with tags, and might write additional unit tests to check a few edge cases, but it looks like this PR is wrapping up, with most of the "fancy" features pushed to another PR.

[CodeLenny]

@auscaster I've also been using JSVerify for testing in some of my other projects. It attempts to find counter-cases when given a property to prove - it only checks random input instead of truly proving the algorithm, but I've found it to be much simpler than building a library of test cases.

I'm not able to spend the time integrating it for this PR, but it's something to consider for later PRs.

[auscaster]

Hey there, just checking in here! I'm sure you've moved on to other things but I would love to see your work merged into master. Is there anything that we need to address before that happens?

[CodeLenny]

@auscaster

• The code currently ignores tags as I only used examples without tags.
• The PR lacks CircleCI testing
• Code hasn't been reviewed, if you wanted to

Not too much left!

[auscaster]

I had a go at getting Circle working for your PR but no luck. If you create a branch of `sourcey/spectacle` and merge the PR into that I think it will work. Regarding tags, they are actually quite important. Paths are combined categorically using tags (you can see it on the demo). If we can maintain that functionality it would be great. Other than that the code looks great and I would be happy to merge it in. Have you been able to implement spectacle successfully for your company yet?

…

On 25 April 2017 at 15:51, Ryan Leonard ***@***.***> wrote: @auscaster <https://github.com/auscaster> - The code currently ignores tags as I only used examples without tags. - The PR lacks CircleCI testing - Code hasn't been reviewed, if you wanted to Not too much left! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGKDN4SVXMnIBbCpVm8uuizmN30apo6ks5rzfprgaJpZM4MRu62> .

[auscaster]

Merging this in. Hope you are well and your projects are coming along nicely Lenny!

[CodeLenny]

@auscaster Sorry I didn't finish it fully, it's been quite a bit crazy here. Thanks for merging!

We've been using Spectacle, although without the remote references at my work, and been loving it.

Side projects have been going well, with the little time I can spend on them :)