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

external references with references are not fully resolved #80

Closed
paulprogrammer opened this issue Feb 24, 2016 · 18 comments
Closed

external references with references are not fully resolved #80

paulprogrammer opened this issue Feb 24, 2016 · 18 comments

Comments

@paulprogrammer
Copy link

Given

root.yaml

top:
  a:
    $ref: "middle.yaml#/definitions/first"
  b:
    $ref: "middle.yaml#/definitions/second"
  c:
    $ref: "middle.yaml#/definitions/third"

middle.yaml

definitions:
  first:
    basic: object
  second:
    $ref: "#/definitions/second-ref"
  third:
    $ref: "bottom.yaml#/definitions/bottom-third"

  second-ref:
    second-object: is-here

bottom.yaml

definitions:
  bottom-third:
     some-other: object-thingy

When requesting a full resolution

Command: json-refs resolve root.yaml

expected

{
  "top": {
    "a": {
      "basic": "object"
    },
    "b": {
      "second-object": "is-here"
    },
    "c": {
      "some-other": "object-thingy"
    }
  }
}

actual

{
  "top": {
    "a": {
      "basic": "object"
    },
    "b": {
      "second-object": "is-here"
    },
    "c": null
  }
}

when requesting an external-only resolution

Command: json-refs resolve -I external root.yaml

I expect the output of a resolution to be internally consistent.

expected

{
  "top": {
    "a": {
      "$ref": "#/definitions/first"
    },
    "b": {
      "$ref": "#/definitions/second"
    },
    "c": {
      "$ref": "#/definitions/third"
    }
  },
  "definitions": {
    "first": {
       "basic": "object"
    },
    "second": {
      "$ref": "/definitions/second-ref"
    },
    "second-ref": {
      "second-object": "is-here"
    },
    "third": {
      "$ref": "/definitions/bottom-third"
    },
    "bottom-third": {
      "some-other": "object-thingy"
    }
}

_actual

{
  "top": {
    "a": {
      "$ref": "middle.yaml#/definitions/first"
    },
    "b": {
      "$ref": "middle.yaml#/definitions/second"
    },
    "c": {
      "$ref": "middle.yaml#/definitions/third"
    }
  }
}

File set: oas-bork.zip

@whitlockjc
Copy link
Owner

How are you running this? Are you running it via the CLI or programmatically?

@paulprogrammer
Copy link
Author

CLI as json-refs resolve root.yaml or json-refs resolve -I external root.yaml

@whitlockjc
Copy link
Owner

Can you run json-refs resolve -I relative -I remote root.yaml? relative and remote are two different filters and it seems like you're only using one.

@paulprogrammer
Copy link
Author

similar to "actual" for full resolution. Still missing the merged definition for second-ref and completely lost the c node data.

{
  "top": {
    "a": {
      "basic": "object"
    },
    "b": {
      "$ref": "#/definitions/second-ref"
    },
    "c": null
  }
}

@whitlockjc
Copy link
Owner

I'll give it a peek.

@whitlockjc
Copy link
Owner

c missing is because you've mixed spaces and tabs so the YAML parser is failing. js-yaml is not giving me any error when it parses but it's clearly having an issue of some kind. I cannot handle this case if js-yaml does not throw an error.

@whitlockjc
Copy link
Owner

Here is how c is parsed:

{
  "definitions": {
    "bottom-third": null
  },
  "some-other": "object-thingy"
}

I'll check to see if there is some sort of configuration to be warned of cases like these but based on what js-yaml is providing, json-refs is doing the right thing. I'm still looking at b.

@whitlockjc
Copy link
Owner

The second issue, b, could be a real bug.

@whitlockjc
Copy link
Owner

Ah...I see what is up. So since we aren't resolving local references, we don't actually attempt to find the #/definitions/second-ref and so we just bring in the reference as it existed. I'm not sure how to handle this.

@whitlockjc
Copy link
Owner

So right now, json-refs is working as expected believe it or not. But it's still broken as you can end up with documents containing references that will not resolve.

@whitlockjc
Copy link
Owner

I'm just not sure how to handle it. If you say "don't resolve local references", local references are left as-is and so they won't be resolved.

@whitlockjc
Copy link
Owner

If you resolve all references, it works just fine once we fix the mixed tab/spaces issues with the files you provided:

~/workspaces/personal/json-refs/bin/json-refs resolve root.yaml                      
{
  "top": {
    "a": {
      "basic": "object"
    },
    "b": {
      "second-object": "is-here"
    },
    "c": {
      "some-other": "object-thingy"
    }
  }
}

I still need to figure out how best to handler options.filter in cases where you don't resolve local references in remote documents.

@paulprogrammer
Copy link
Author

re: tabs mixed w/ spaces doh

Re other: resolving external references should merge internal references from the external files so that the $ref elements are resolvable. I recognize this causes a problem with potential name collisions. I have some ideas; will say more here later.

@paulprogrammer
Copy link
Author

So here's my thoughts:

  1. you need to know enough about the state of the parser to know if you're parsing the root file or a subordinate referenced file.
  2. When you find an internal reference in a file you need to follow that reference to find the object associated to the reference (this needs to be done exactly once per reference in a given file)
  3. You have to mangle the reference to avoid name collisions. I recommend sandwiching the file name between /definitions and the rest of the path.
  4. You then merge the object in with the output dictionary.

Thus the example becomes:

top:
  a:
    basic: object
  b:
    $ref: "#/definitions/middle.yaml/second-ref"
  c:
   $ref: "#/definitions/bottom.yaml/bottom-third"

definitions:
  middle.yaml:
    second-ref:
      second-object: is-here
  bottom.yaml:
    bottom-third:
     some-other: object-thingy

Some alternates:

  1. remove # from the ref-path, and prepend /definitions/ to it such that middle.yaml#/definitions/second-ref becomes /definitions/middle.yaml/definitions/second-ref

@paulprogrammer
Copy link
Author

It becomes very much like a real compiliation where you keep a symbol table of references and resolve them only if they're not found in the symtab. This is what I was starting to do in python before you generously offered to update json-refs. :)

@whitlockjc
Copy link
Owner

The problem isn't that we're not keeping track of state, it's that we resolve state based on what is requested and if you don't ask json-refs to resolve local references, we don't do it. This means that if you have a remote reference to a path in a document that itself has local references within said document, we don't resolve them because you've asked us not to. Since full resolution works just fine, this isn't a state issue. This is a "we need to rethink how options.filter works for relative/remote documents" issue.

@paulprogrammer
Copy link
Author

Maybe it's just what you consider internal vs. external references. An internal reference inside an external file ought to be considered an external reference because it's external in reference to the root. Thoughts?

In other words, the state stack can promote from internal -> external -> remote, but it can never demote from e.g. external -> internal?

diegode pushed a commit to medallia/json-refs that referenced this issue Sep 12, 2016
diegode pushed a commit to medallia/json-refs that referenced this issue Sep 12, 2016
diegode pushed a commit to medallia/json-refs that referenced this issue Sep 12, 2016
diegode pushed a commit to medallia/json-refs that referenced this issue Sep 13, 2016
@diegode diegode mentioned this issue Sep 13, 2016
@diegode
Copy link

diegode commented Sep 13, 2016

Please review #94

whitlockjc added a commit that referenced this issue Apr 20, 2017
The resolver was rewritten to fix numerous bugs and performance issues.
The reason for this is over the years, bug fixes and features have been
added on top of json-refs and its resolver logic has become crufty.  The
new rewrite is clean, does not reinvent the wheel and fixes many bugs.

Fixes: #80, #87, #88, #89, #97, #100, #101, #103
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

Successfully merging a pull request may close this issue.

3 participants