Add jsonschema $ref support

[coltonlw]

This PR adds support for $ref's in jsonschema, a widely used feature in API spec's

[coltonlw]

Can someone with more nodejs experience please help me figure out why these two tests are failing? I would love to fix them and contribute this important feature to abao https://travis-ci.org/cybertk/abao/jobs/152697778#L851

[plroebuck]

So I'm confused. The package we're already using supports $refs, so why exactly is this PR needed?

[coltonlw]

This package does not already support $ref's as defined in the jsonschema spec. Currently you have to pass a glob pattern to include all of the jsonschema files you want to $ref from the filesystem, and if they are in different directories it doesn't work at all.

On Tue, Aug 16, 2016 at 11:10 PM, plroebuck notifications@github.com wrote:

So I'm confused. The package we're already using supports $refs, so why exactly is this PR needed?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cybertk/abao/pull/163#issuecomment-240308753, or mute the thread https://github.com/notifications/unsubscribe-auth/AT8Lk8S35sI7MzfmpiaSgyzZzbuDe9JNks5qgom2gaJpZM4JleNm .

Colton Leekley-Winslow Backend Developer Flywheel.io https://flywheel.io/

[plroebuck]

Perhaps our cmdline arg scheme would need be modified to handle multiple directories, but why not use the existing package's method tv4.addSchema() for handling $refs?

[coltonlw]

Because it can't handle automatically resolving $ref's to the filsystem and if a project is using that functionality as allowed in the jsonschema spec, manually configuring addSchema() or multiple directories becomes messy quickly

On Wed, Aug 17, 2016 at 8:16 AM, plroebuck notifications@github.com wrote:

Perhaps our cmdline arg scheme would need be modified to handle multiple directories, but why not use the existing package's method tv4.addSchema() for handling $refs?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cybertk/abao/pull/163#issuecomment-240407185, or mute the thread https://github.com/notifications/unsubscribe-auth/AT8LkyWhpsAZxicyI1MmK2_BAqGilGHvks5qgwmUgaJpZM4JleNm .

Colton Leekley-Winslow Backend Developer Flywheel.io https://flywheel.io/

[coltonlw]

Why wouldn't you want this project to support resolving $ref's as defined in the spec?

On Wed, Aug 17, 2016 at 8:23 AM, Colton Leekley-Winslow < coltonlw@flywheel.io> wrote:

Because it can't handle automatically resolving $ref's to the filsystem and if a project is using that functionality as allowed in the jsonschema spec, manually configuring addSchema() or multiple directories becomes messy quickly

On Wed, Aug 17, 2016 at 8:16 AM, plroebuck notifications@github.com wrote:

Perhaps our cmdline arg scheme would need be modified to handle multiple directories, but why not use the existing package's method tv4.addSchema() for handling $refs?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cybertk/abao/pull/163#issuecomment-240407185, or mute the thread https://github.com/notifications/unsubscribe-auth/AT8LkyWhpsAZxicyI1MmK2_BAqGilGHvks5qgwmUgaJpZM4JleNm .

Colton Leekley-Winslow Backend Developer Flywheel.io https://flywheel.io/

Colton Leekley-Winslow Backend Developer Flywheel.io https://flywheel.io/

[plroebuck]

As we resolve $refs already using this method, I am asking why we cannot continue to do so (by making additional calls to it with extra directories given by cmdline args).

[coltonlw]

Because then a project using $ref's has to maintain that configuration which is not compliant with the jsonschema spec. By using json-schema-ref-parser to parse the $ref's, projects using filesystem $ref's will work out of the box with abao, instead of requiring this additional non-standard configuration

On Wed, Aug 17, 2016 at 8:36 AM, plroebuck notifications@github.com wrote:

As we resolve $refs already using this method, I am asking why we cannot continue to do so (by making additional calls to it with extra directories given by cmdline args).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cybertk/abao/pull/163#issuecomment-240412835, or mute the thread https://github.com/notifications/unsubscribe-auth/AT8Lk0MDAOPxKRKDLVwI7_d0V2SKTuwfks5qgw5qgaJpZM4JleNm .

Colton Leekley-Winslow Backend Developer Flywheel.io https://flywheel.io/

[plroebuck]

Telling tv4 where the $refs live does not make it "not compliant with the spec" -- it tells it how to interpret the spec. IMO, using a PATH-like variable to specify the locations provides the same functionality. Don't follow your "has to maintain this configuration" argument. You're maintaining it either via cmdline or RAML itself. Using a cmdline solution is always more flexible than hardcoded one.

[coltonlw]

According to the spec, $ref's that are a relative path are resolved relative to the path of the referrer, so ignoring that and requiring the user to specify how to resolve those $ref's is most certainly not compliant with the spec. In regards to maintaining the configuration, the information necessary to resolve the $ref's is already available if using a proper parser, and if you already have a working $ref configuration I'm not sure what benefit you would get by being able to override that via the command line.

Here jsonschema core spec says to use json-reference spec for behavior of references: http://json-schema.org/latest/json-schema-core.html#anchor25

In the json-reference document they clearly state $ref's are resolved relative to the referring document https://tools.ietf.org/html/draft-pbryan-zyp-json-ref-03#section-4

On Wed, Aug 17, 2016 at 8:59 AM, plroebuck notifications@github.com wrote:

Telling tv4 where the specs live does not make it "not compliant with the spec" -- it tells it how to interpret the spec. IMO, using a PATH-like variable to specify the locations provides the same functionality. Don't follow your "has to maintain this configuration" argument. You're maintaining it either via cmdline or RAML itself. Using a cmdline solution is always more flexible than hardcoded one.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cybertk/abao/pull/163#issuecomment-240419751, or mute the thread https://github.com/notifications/unsubscribe-auth/AT8LkynmMxPjVI7ICNmo6FJOoVzw78Meks5qgxPQgaJpZM4JleNm .

Colton Leekley-Winslow Backend Developer Flywheel.io https://flywheel.io/

[plroebuck]

Please provide a sample RAML that illustrates your issue.

[coltonlw]

Absolutely, thanks for taking your time to review this

https://github.com/scitran/core/tree/master/raml

[coltonlw]

A good example is this schema in output schemas which includes a schema from our shared schemas
https://github.com/scitran/core/blob/masater/raml/schemas/output/user.json

[cybertk]

Thanks for the explaining from both of you.

The current abao support deference jsonschema $ref with -s options (For @coltonlw 's example, it's abao -s raml/schemas/**/*.json)

The implementation details can be found in TestFactory, TestFactory will load all schema files defined by -s with tv4.addSchema()

For this new PR,

I think we changed the way how we handle schemas, the PR use json-schema-ref-parser to deference schemas and generate a new large schema file which already resolved all $refs, so the tv4.addSchema() is unnecessary in this case.

IMO, I prefer this PR's mechanism to handle $ref, as it can handles them automatically. But if we use this PR, we need remove the previous implementation as they do the same thing.

What's your suggestion?

[coltonlw]

You could merge the PR and continue support both mechanisms (they work side-by-side), and then also add a deprecation warning message the user sees if they specify "-s". That will give people time to upgrade incase they are using "-s" for ref's that would otherwise be unresolvable.

[coltonlw]

I'm still curious about these failing tests though, any help there is appreciated

[cybertk]

@coltonlw There are two errors

1. #assertResponse - when response body is null - should throw AssertionError

It's caused by your new code, use assert.xx instead of throw xx

1. when invoked with "--schema" option and expecting error - exit status should be 1

It caused by invalid URL in fixture with-json-refs.json, as json-schema-ref-parser tried to download it. You may replace it with a local one.