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

Error is thrown on duplicate keys in resolved remote references #137

Closed
MikeRalphson opened this issue Mar 30, 2017 · 7 comments
Closed

Comments

@MikeRalphson
Copy link
Contributor

MikeRalphson commented Mar 30, 2017

I believe it is this line responsible.

The error finally reported is not very indicative of the source of the problem:

  • code: UNRESOLVABLE_REFERENCE
    error: ptr must be a JSON Pointer
    message: 'Reference could not be resolved: ../examples/NameSpaces/SBNameSpaceList.json'
    path:
    - paths
    - '/subscriptions/{subscriptionId}/providers/Microsoft.ServiceBus/namespaces'
    - get
    - x-ms-examples
    - NameSpaceList
    - $ref

Would you accept a PR to add options of {json: true} to the YAML.safeLoad() call, possibly controlled by an option setting?

Real life example (this PR refers).

https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/arm-servicebus/2015-08-01/swagger/servicebus.json

@whitlockjc
Copy link
Member

This is a duplicate of #131.

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Mar 30, 2017

This is a duplicate of #131.

Only partially. The clobbering of the json-refs error message is one thing, but treating all resolved JSON content as strict YAML breaks the defined behaviour of JSON in that duplicate keys are ignored.

@whitlockjc
Copy link
Member

js-yaml should be throwing an error whenever it attempts to load a document with duplicate keys: nodeca/js-yaml@9ea29ec I checked the release version and it's older than the one we're using. I will do a quick check with json-refs to see if it's suppressing the error, a known issue.

@MikeRalphson
Copy link
Contributor Author

Thanks for the update. In my case the data being returned is JSON not YAML, and so the js-yaml.safeLoad() in strict mode is producing different results than a JSON.parse() would return. I still consider this less than optimal behaviour.

@whitlockjc
Copy link
Member

I've tracked it down. There are three players here:

  • js-yaml
  • json-refs
  • sway

Here are the results:

Test YAML

---
definitions:
  Person:
    properties:
      age:
        type: integer
      age:
        type: string

Test JS

var fs = require('fs');
var JsonRefs = require('json-refs');
var Sway = require('sway');
var YAML = require('js-yaml');

var docLocation = '/tmp/test.yaml';

function printBad (name, err) {
  console.error('Bad document (%s): %s', name, err.message);
}

function printGood (name) {
  console.log('Good document (%s)', name);
}

try {
  YAML.safeLoad(fs.readFileSync(docLocation, 'utf8'));

  printGood('js-yaml');
} catch (err) {
  printBad('js-yaml', err)
}

JsonRefs.findRefsAt(docLocation)
  .then(function () {
    printGood('json-refs');
  })
  .catch(function (err) {
    printBad('json-refs', err);
  });

Sway.create({definition: docLocation})
  .then(function () {
    printGood('sway');
  })
  .catch(function (err) {
    printBad('sway', err);
  });

Test Output

Bad document (js-yaml): duplicated mapping key at line 7, column -25:
          age:
          ^
Bad document (json-refs): Unexpected number in JSON at position 1
Bad document (sway): duplicated mapping key at line 7, column -25:
          age:
          ^

Results

It seems that if the duplicate key is in the root document, js-yaml and sway handle it just fine while json-refs seems to have some weird issue (not sure what is up but I'll report it there and link this issue). But, if the duplicate key is not in the root document and is instead in an included document, then json-refs is the one processing the document so it would be impacted by the weird bug.

As for the JSON vs. YAML portion of your last response, it's already reported here: whitlockjc/json-refs#101 Long story short, I wanted to avoid having to juggle parsing JSON vs. YAML and since js-yaml successfully parses both, it seemed like a quick win but it's not 100%.

@MikeRalphson
Copy link
Contributor Author

As for the JSON vs. YAML portion of your last response, it's already reported here: whitlockjc/json-refs#101 Long story short, I wanted to avoid having to juggle parsing JSON vs. YAML and since js-yaml successfully parses both, it seemed like a quick win but it's not 100%.

Understand. At the moment APIs.guru requires the use of a sway patched as per the original suggestion in this issue, but I don't really want to have to maintain forks of api-spec-converter and sway. But I can put off a decision on switching to another validator for a while until the situation on support for OpenAPI 3 becomes clearer.

@whitlockjc
Copy link
Member

PRs are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants