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

Schema validation doesn't work with YAML Aliases #183

Closed
tedepstein opened this issue Aug 17, 2016 · 8 comments
Closed

Schema validation doesn't work with YAML Aliases #183

tedepstein opened this issue Aug 17, 2016 · 8 comments
Assignees

Comments

@tedepstein
Copy link
Collaborator

This problem was reported by a user in support ticket #150.

YAML supports anchors and aliases, which allow nodes to be repeated in the YAML structure.

This Swagger spec should be allowed, but fails validation:


---
swagger: "2.0"
info:
  version: 1.0.0
  title: Uber parameters

paths: {}

parameters:  

  law:
    name: law_applicability
    in: query
    description: scope of a law
    required: false
    type: string
    &scope_values enum: 
    - GLOBAL
    - REGIONAL
    - LOCAL

  brand:
    name: brand_reach
    in: query
    description: where a brand name is used
    type: string
    enum:
      *scope_values

The error is:

Failed to match exactly one schema:
 - nonBodyParameter:
    Failed to match exactly one schema:
     - queryParameterSubSchema:
         - value of type string is not allowed, value should be of type array
     - headerParameterSubSchema:
         - value query is not allowed, value should be one of "header"
         - value of type string is not allowed, value should be of type array
     - formDataParameterSubSchema:
         - value query is not allowed, value should be one of "formData"
         - value of type string is not allowed, value should be of type array
     - pathParameterSubSchema:
         - object has missing required properties "required"
         - value query is not allowed, value should be one of "path"
         - value of type string is not allowed, value should be of type array
 - bodyParameter:
     - object has properties "enum", "type" which are not allowed
     - value query is not allowed, value should be one of "body"
     - object has missing required properties "schema"
@tfesenko
Copy link
Member

tfesenko commented Aug 18, 2016

A better usage of the aliases in this case would be

swagger: "2.0"
info:
  version: 1.0.0
  title: Uber parameters

paths: {}

parameters:  

  law:
    name: law_applicability
    in: query
    description: scope of a law
    required: false
    type: string
    enum: &scope_values
    - GLOBAL
    - REGIONAL
    - LOCAL

  brand:
    name: brand_reach
    in: query
    description: where a brand name is used
    type: string
    enum: *scope_values

Online Swagger editor converts it to the following JSON:

{
    "swagger": "2.0",
    "info": {
        "version": "1.0.0",
        "title": "Uber parameters"
    },
    "paths": {},
    "parameters": {
        "law": {
            "name": "law_applicability",
            "in": "query",
            "description": "scope of a law",
            "required": false,
            "type": "string",
            "enum": [
                "GLOBAL",
                "REGIONAL",
                "LOCAL"
            ]
        },
        "brand": {
            "name": "brand_reach",
            "in": "query",
            "description": "where a brand name is used",
            "type": "string",
            "enum": [
                "GLOBAL",
                "REGIONAL",
                "LOCAL"
            ]
        }
    }
}

while the originally posted spec is not expanded correctly by the online Swagger editor:

        "brand": {
            "name": "brand_reach",
            "in": "query",
            "description": "where a brand name is used",
            "type": "string",
            "enum": "enum"
        }

But even this spec causes the same error as the one reported in the issue description.

The reason is that com.reprezen.swagedit.editor.SwaggerEditor.validateSwagger(IFile, SwaggerDocument, IFileEditorInput) fails. Inside this method we

  1. convert our YAML spec to JSON (kept in jsonSpec: com.fasterxml.jackson.databind.JsonNode variable) and
  2. validate jsonSpec again JSON Schema for Swagger:
validateAgainstSchema(new ErrorProcessor(yaml), jsonContent)

The problem is that the our jsonSpec does not expand aliases, note "enum": "scope_values" in

{
  "swagger": "2.0",
  "info": {
    "version": "1.0.0",
    "title": "Uber parameters"
  },
  "paths": {},
  "parameters": {
    "law": {
      "name": "law_applicability",
      "in": "query",
      "description": "scope of a law",
      "required": false,
      "type": "string",
      "enum": [
        "GLOBAL",
        "REGIONAL",
        "LOCAL"
      ]
    },
    "brand": {
      "name": "brand_reach",
      "in": "query",
      "description": "where a brand name is used",
      "type": "string",
      "enum": "scope_values"
    }
  }
}

We use the standard Swagger YAML ObjectMapper: io.swagger.util.Yaml.mapper()
And Jackson supports aliases in YAML:

So the goal is to get a proper jsonSpec to feed it to the schema validator.
@andylowry wrote:

The Swagger parser does this correctly, so it handles aliases as long as there's no recursion (and I don't think there's anywhere in Swagger where alias recursion could possibly make sense).

So there should be a way to get a right jsonSpec.

@tfesenko
Copy link
Member

tfesenko commented Aug 18, 2016

Note that if we reorder parameters and put the anchor after the referencing alias it will show a different error:

null; found undefined alias scope_values; in 'reader', line 14, column 11:
enum: *scope_values

Spec:

swagger: "2.0"
info:
  version: 1.0.0
  title: Uber parameters

paths: {}

parameters:  
  brand:
    name: brand_reach
    in: query
    description: where a brand name is used
    type: string
    enum: *scope_values 
  law:
    name: law_applicability
    in: query
    description: scope of a law
    required: false
    type: string
    enum: &scope_values
    - GLOBAL
    - REGIONAL
    - LOCAL

This error is produced by validateYaml(). We use Snake-YAML 1.12 to validate it. This needs investigation too. We may need to switch to a newer version.

@tedepstein
Copy link
Collaborator Author

tedepstein commented Aug 18, 2016

@tfesenko , I think it may be a requirement in YAML to declare the anchor before the alias.

Please note the discussion of serialization order here and here.

@tedepstein
Copy link
Collaborator Author

tedepstein commented Aug 18, 2016

Ted asked,

Andy, have you seen this kind of syntax before?

@andylowry wrote:

Yes, it's definitely legit in YAML, though I've never seen anyone use it in swagger. I'm not sure the Swagger folks would endorse this use. Here's what the swagger spec says:

The files describing the RESTful API in accordance with the Swagger specification are represented as JSON objects and conform to the JSON standards. YAML, being a superset of JSON, can be used as well to represent a Swagger specification file.

So my sense is that JSON is the language that Swagger specs must adhere to, but YAML is accepted so long as it's limited to something that corresponds to well-formed JSON. There is no concept like aliases in JSON itself, though JSON Pointers accomplish the same thing (considering only simple aliases - YAML also allows alias references to provide local modifications to the reference value). A YAML processor could inline alias references and, as long as there were no recursive references, the result would be a JSON-compliant result (ignoring other JSON-incompatible YAML feature like type tags).

Jackson does handle aliases in its YAML processing. It was implemented using the previously existing object-sharing facility that's used for the @JsonObjectId annotation. That means that aliases in YAML are deserialized into Java object structures with shared references to aliased sub-structures. Recursion is not a problem until you try to traverse the structure, unless you're aware of this sharing. But it appears that the approach to deserialization needs to explicitly use the snakeyaml loader and then convert that result to a jackson JsonNode structure.

The Swagger parser does this correctly, so it handles aliases as long as there's no recursion (and I don't think there's anywhere in Swagger where alias recursion could possibly make sense). The normalizer does not, so it fails on aliases, period. The needed alterations to normalizer are very straightforward.

What all this means for SwagEdit is not something I can comment on.

@tedepstein
Copy link
Collaborator Author

tedepstein commented Aug 18, 2016

@andylowry , for the current scope, we only need to look at simple aliases, and we don't need to accommodate any kind of recursion.

The problems in SwagEdit are limited to validation, as Tanya described. And for that, we explicitly convert YAML to JSON. Aliases should be dereferenced and expanded in-place for that conversion; we are not trying to preserve the topology of the YAML node graph.

YAML also allows alias references to provide local modifications to the reference value

I didn't see that kind of override mechanism described in YAML, and in fact it looked to me like it's explicitly disallowed. Where you're using an alias, you can't also have properties and values. But If I'm misunderstanding something, please point me to the relevant section of the YAML spec.

The normalizer does not, so it fails on aliases, period. The needed alterations to normalizer are very straightforward.

Thanks, I will open a separate issue for this in Jira.

@tedepstein
Copy link
Collaborator Author

tedepstein commented Aug 18, 2016

@tfesenko , from what I've heard so far, this seems like it should be fixable without too much effort. Maybe @andylowry's comment will help point us in the right direction:

...it appears that the approach to deserialization needs to explicitly use the snakeyaml loader and then convert that result to a jackson JsonNode structure.

Based on our preliminary assessment, I'm going to mark this high-priority. Hoping we'll be able to fix this within the next few business days.

@tfesenko tfesenko self-assigned this Aug 18, 2016
tfesenko added a commit that referenced this issue Aug 18, 2016
1. Aliases are correctly dereferenced and expanded in-place by
SnakeYaml. 
2. Then we convert the object (usually it's a `Map`) to a string, which
is passed to Jackson's Mapper.

I hope to find a better way of transforming Snake-YAML results into a
`JsonNode`
tfesenko added a commit that referenced this issue Aug 19, 2016
@tfesenko
Copy link
Member

It would be good to provide navigation (Ctrl+click) from an alias to the corresponding anchor.

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

No branches or pull requests

2 participants