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

JSON References aren't properly URL-decoded #72

Closed
glen-84 opened this issue Apr 23, 2016 · 13 comments
Closed

JSON References aren't properly URL-decoded #72

glen-84 opened this issue Apr 23, 2016 · 13 comments

Comments

@glen-84
Copy link

glen-84 commented Apr 23, 2016


Swagger Editor erroneously shows "Reference could not be resolved" errors for JSON references that contain URL-encoded characters. For example, "#/definitions/foo%20bar" instead of "#/definitions/foo bar". According to RFC 6901, sections 5 & 6, JSON pointers should be URL-encoded.

Here's a simplistic example: https://gist.github.com/BigstickCarpet/5ad1a760acbb598d38c5dbba223eb9bd

And here's a more real-world example: https://gist.github.com/BigstickCarpet/e867ccd425e730634ee71f43c462a34f

As far as I know, Swagger Editor uses json-refs under the hood to do its JSON Pointer decoding. json-refs has the correct logic for decoding URL-encoded JSON Pointers, so maybe Swagger Editor is just using an older version of json-refs? Maybe @whitlockjc can provide more info.


Copied from swagger-api/swagger-editor#871, as this is apparently an issue with Sway.

/cc @BigstickCarpet

@whitlockjc
Copy link
Member

whitlockjc commented Apr 24, 2016

json-refs properly resolves all references in the examples provided by @BigstickCarpet. So json-refs is not the issue, unless JsonRefs#pathToPtr representation is wrong which we'll discuss later. (Now that json-refs has a CLI, one can easily check these things using npm install -g json-refs && json-refs resolve SOME_PATH_OR_URL.)

The problem is Sway is attempting to use the raw $ref value, in its encoded glory, when building an internal model of referenceable objects and their references. This is where things get messed up. If Sway sees #/definitions/foo%20bar, it looks for a referenceable object with that URI but since json-refs outputs JSON Pointers with its URIs decoded, there is a mismatch. So there's two options here:

  1. Have Sway check for encoded and decoded URIs
  2. Have json-refs output URIs as URI encoded values

@BigstickCarpet points out that JSON Pointers are suppose to be URI encoded and I'm not sure I read the same but I will revisit it.

@JamesMessinger
Copy link

JamesMessinger commented Apr 24, 2016

I originally posted this issue for Swagger Editor, not Sway (original issue here). As mentioned there, I think json-refs has the correct logic for decoding URL-encoded JSON Pointers, so that's not the problem. It's likely that Swagger Editor is just using an older version of json-refs.

To reproduce the bug, just paste either of my examples above at http://editor.swagger.io

@whitlockjc
Copy link
Member

whitlockjc commented Apr 24, 2016

I understand but since swagger-editor is getting json-refs from sway and json-refs fully resolves all references properly, I'm not sure how json-refs itself is the culprit. (json-refs is potentially the culprit for the false warning but I think since json-refs handles encoded/decoded URIs, sway should too and option 1 suggested above is my suggested fix.) What I am seeing is that sway is reporting an erroneous Definition is not used: #/definitions/foo bar warning but it is not reporting the Reference could not be resolved: #/definitions/foo%20bar error.

To further explain what I mean, if I take either of your documents and use the following code, neither report the Reference could not be resolved: #/definitions/foo%20bar error you are presented with from swagger-editor:

'use strict'


var JsonRefs = require('json-refs');
var Sway = require('sway');

function processResults (header, results) {
  console.log('%s: %s', header, results.length);

  results.forEach(function (result) {
    console.log('  %s: %s', JsonRefs.pathToPtr(result.path), result.message);
  });

  console.log();
}

Sway.create({
  definition: './testing.yaml'
})
  .then(function (api) {
    var results = api.validate();

    processResults('Errors', results.errors);
    processResults('Warnings', results.warnings);
  })
  .catch(function (err) {
    console.error(err);
  });

When I run this against your first example, here is the output:

node testing.js       
Errors: 0

Warnings: 1
  #/definitions/foo bar: Definition is not used: #/definitions/foo bar

So if json-refs is resolving things properly and sway is only reporting the warning (erroneously I agree), where is this other error coming from?

@whitlockjc
Copy link
Member

Copying @saharj.

@whitlockjc
Copy link
Member

Just to be complete, here is an example of json-refs handling these references properly:

~/workspaces/personal/json-refs/bin/json-refs resolve testing.yaml --yaml 
swagger: '2.0'
info:
  version: 1.0.0
  title: Example
paths:
  /foo:
    get:
      responses:
        '200':
          description: Success
          schema:
            type: string
definitions:
  foo bar:
    type: string

Had json-refs failed to resolve any reference, it would had croaked and you would had seen an error like this:

~/workspaces/personal/json-refs/bin/json-refs resolve testing.yaml --yaml                                                                                                                                               146 ↵

  error: Document has invalid references:

  #/paths/~1foo/get/responses/200/schema: JSON Pointer points to missing location: #/definitions/fool bar

@glen-84
Copy link
Author

glen-84 commented May 18, 2016

Any news regarding this issue?

@glen-84
Copy link
Author

glen-84 commented Sep 10, 2016

@whitlockjc Do you think that you'll be able to look into this soon?

@whitlockjc
Copy link
Member

Yes. I haven't had a lot of time recently to put into json-refs but I will do my best to get something out this week.

@glen-84
Copy link
Author

glen-84 commented Feb 1, 2017

Which week? 😁 Just kidding.

@whitlockjc
Copy link
Member

Just to summarize what I think is necessary:

  1. json-refs needs to handle URI encoded JSON Pointers/References as inputs
  2. json-refs needs to output JSON Pointers/References as URI encoded

I think earlier I mentioned this as an either/or and I don't think that is accurate. Can someone confirm?

@glen-84
Copy link
Author

glen-84 commented Jan 27, 2018

Can someone confirm?

I honestly don't know the details. I think that @BigstickCarpet would have to confirm.

@whitlockjc
Copy link
Member

First pass at this one is a pain. I can reproduce it of course but the internal model for circulars/dependencies makes it painful. I'll keep at it.

@whitlockjc
Copy link
Member

This was fixed in json-refs@v3.0.4 and committed the fix in cdd8030

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

3 participants