apiaryio / dredd

Language-agnostic HTTP API Testing Tool
https://dredd.org
MIT License
4.2k stars 280 forks source link

`null` as a string value in JSON breaks tests #194

Closed neonichu closed 8 years ago

neonichu commented 9 years ago

Since null is a valid JSON value for a string, it should also be accepted by dredd.

Failure output from the test run:

fail: GET /spaces/5smsq22uwt4m/webhook_definitions/foobar duration: 552ms
fail: body: At '/httpBasicUsername' Invalid type: null (expected string)

request: 
body: 

headers: 
    Authorization:  Bearer XXXXX
    User-Agent: Dredd/0.5.2 (Darwin 14.4.0; x64)

uri: /spaces/5smsq22uwt4m/webhook_definitions/foobar
method: GET

expected: 
headers: 
    Content-Type: application/vnd.contentful.management.v1+json

body: 

statusCode: 200
bodySchema: {"type":"object","properties":{"sys":{"type":"object","properties":{"type":{"type":"string"},"id":{"type":"string"},"version":{"type":"number"},"space":{"type":"object","properties":{"sys":{"type":"object","properties":{"type":{"type":"string"},"linkType":{"type":"string"},"id":{"type":"string"}}}}},"createdBy":{"type":"object","properties":{"sys":{"type":"object","properties":{"type":{"type":"string"},"linkType":{"type":"string"},"id":{"type":"string"}}}}},"createdAt":{"type":"string"},"updatedBy":{"type":"object","properties":{"sys":{"type":"object","properties":{"type":{"type":"string"},"linkType":{"type":"string"},"id":{"type":"string"}}}}},"updatedAt":{"type":"string"}}},"url":{"type":"string"},"httpBasicUsername":{"type":"string"}},"$schema":"http://json-schema.org/draft-04/schema#"}

actual: 
statusCode: 200
headers: 
    accept-ranges: bytes
    access-control-allow-headers: Accept,Accept-Language,Authorization,Cache-Control,Content-Length,Content-Range,Content-Type,DNT,Destination,Expires,If-Match,If-Modified-Since,If-None-Match,Keep-Alive,Last-Modified,Origin,Pragma,Range,User-Agent,X-Http-Method-Override,X-Mx-ReqToken,X-Requested-With,X-Contentful-Version,X-Contentful-Content-Type,X-Contentful-Organization,X-Contentful-Skip-Transformation
    access-control-allow-methods: DELETE,GET,HEAD,POST,PUT,OPTIONS
    access-control-allow-origin: *
    access-control-expose-headers: Etag
    access-control-max-age: 1728000
    cache-control: max-age=0
    content-type: application/vnd.contentful.management.v1+json
    date: Wed, 20 May 2015 13:08:33 GMT
    etag: "6fdbb274e06d917293140ba5a93edc53"
    server: nginx
    status: 200 OK
    x-contentful-request-id: 9f3-1410702540
    content-length: 621
    connection: keep-alive

body: 
{
  "sys":{
    "type":"WebhookDefinition",
    "id":"foobar",
    "version":0,
    "space":{
      "sys":{
        "type":"Link",
        "linkType":"Space",
        "id":"5smsq22uwt4m"
      }
    },
    "createdBy":{
      "sys":{
        "type":"Link",
        "linkType":"User",
        "id":"4FLrUHftHW3v2BLi9fzfjU"
      }
    },
    "createdAt":"2015-05-20T12:01:00Z",
    "updatedBy":{
      "sys":{
        "type":"Link",
        "linkType":"User",
        "id":"4FLrUHftHW3v2BLi9fzfjU"
      }
    },
    "updatedAt":"2015-05-20T12:01:00Z"
  },
  "url":"https://www.example.com",
  "httpBasicUsername":null
}
netmilk commented 9 years ago

Hi @neonichu,

thanks a lot for reporting this issue and sorry for late response.

Can you please submit the evidence blueprint? I'll try to find some workaround. I suspect that you are using Body Attributes section, where is specified that the value of the httpBasicUsername key should be a string. In that case simple null is not valid value, because null is primitive type null not string.

But problem is that as far as I know there is no way how to describe null type in the MSON which is used for Body Attributes description in the blueprint.

I reported it here.

Adam

neonichu commented 9 years ago

The MSON I use looks like this:

### Webhook Definition

- sys
    - type: WebhookDefinition
    - id: foobar
    - version: 0 (number)
    - space (Space Link)
    - Include Resource Metadata
- url: https://www.example.com
- httpBasicUsername

As far as I understood it, leaving out the value leads to null in the JSON, see https://github.com/apiaryio/mson/blob/46081361555a77f2085b89d53b1418a982c10122/README.md#objects--arrays

netmilk commented 9 years ago

Interesting. It seems that it's a drafter.js or drafter issue. Drafter takes parsed MSON AST and transforms it into a JSON schema representation which Dredd with use of Gavel uses for validation of the body. But generated JSON schema doesn't validate values, only their types.

This is JSON schema generated from your MSON. When there is no type given in the MSON it should definitely not expect that value is a string.

I reported it here

{  
   "type":"object",
   "properties":{  
      "sys":{  
         "type":"object",
         "properties":{  
            "type":{  
               "type":"string"
            },
            "id":{  
               "type":"string"
            },
            "version":{  
               "type":"number"
            },
            "space":{  
               "type":"object",
               "properties":{  
                  "sys":{  
                     "type":"object",
                     "properties":{  
                        "type":{  
                           "type":"string"
                        },
                        "linkType":{  
                           "type":"string"
                        },
                        "id":{  
                           "type":"string"
                        }
                     }
                  }
               }
            },
            "createdBy":{  
               "type":"object",
               "properties":{  
                  "sys":{  
                     "type":"object",
                     "properties":{  
                        "type":{  
                           "type":"string"
                        },
                        "linkType":{  
                           "type":"string"
                        },
                        "id":{  
                           "type":"string"
                        }
                     }
                  }
               }
            },
            "createdAt":{  
               "type":"string"
            },
            "updatedBy":{  
               "type":"object",
               "properties":{  
                  "sys":{  
                     "type":"object",
                     "properties":{  
                        "type":{  
                           "type":"string"
                        },
                        "linkType":{  
                           "type":"string"
                        },
                        "id":{  
                           "type":"string"
                        }
                     }
                  }
               }
            },
            "updatedAt":{  
               "type":"string"
            }
         }
      },
      "url":{  
         "type":"string"
      },
      "httpBasicUsername":{  
         "type":"string"
      }
   },
   "$schema":"http://json-schema.org/draft-04/schema#"
}
nevir commented 9 years ago

As a workaround, would it work to have gavel strip all null properties from JSON responses (maybe as an option)?

I'd be interested in making a patch for that, but I'm having a heck of a time tracing through gavel's validation logic to find the right point for that

netmilk commented 9 years ago

@nevir Thanks for proposing this solution! My humble opinion is that it's supposed to be not a workaround in Gavel but in Dredd.

It's interesting, I was initially going to recommend to achieve this without hacking Dredd by writing a beforeEach hook, where you can traverse the parsed response object and modify it by striping keys with null values or convert them to strings.

But then I realised it needs the server response to be modified before the validation which is not supported in Dredd at this moment. So the proper workaround is to write support for beforeEachValidation and beforeValidation hooks in Dredd and then write appropriate hook :) Something like:

traverse = require('traverse');
hooks = require('hooks');

hooks.beforeEachValidation(function(transaction){
  var response = JSON.parse(transaction.real.body);
  var newResponse = traverse(response).map(function(){
    if(this.node === null){
      this.update(String(this.node));
    };
  transaction.real.body = newResponse.stringify()
});

I created issue #221 for support for this types of hooks.

netmilk commented 9 years ago

Stressed in Drafter.js issues

obihann commented 9 years ago

Just a note that their is now a pull request in the MSON specifications to allow null as a type - apiaryio/dredd/issues/194

lippea commented 8 years ago

I have the same problem in dredd 2.14.12. Shall I go for the workaround or is there a fix in another version?

honzajavorek commented 8 years ago

@lippea AFAIK this shouldn't be an issue anymore, we just forgot to close it. The behavior should be, as far as my knowledge goes, following:

E.g. See Dredd's tests or MSON spec.

You noted that you are using 2.14.12 version of Dredd, but that's not possible, since currently the latest Dredd version is 1.5.0. Could you please file another issue, which would exactly describe what is your problem, your input and what's expected? Thanks!