apiaryio / dredd

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

Array member objects are not validated by Gavel.js #177

Open netmilk opened 9 years ago

netmilk commented 9 years ago

Dredd, Gavel.js respectively is not aware of array members type and structure and it should be.

For example presence of key "key" in following payload example is never validated.

[
  {
    "key": "value"
  }
] 
ecordell commented 9 years ago

To clarify - this is only true if no schema is provided, correct? (i.e. the issue is with the automatic schema generation and not validation?)

netmilk commented 9 years ago

@ecordell Of course, yes, you're right! :) This is valid only for automatic expectations based on example JSONs.

jolleon commented 9 years ago

Hi, I'm seeing the issue with array members not getting validated also when using a Data Structure.

For instance consider this blueprint:

# Group Users

## Users [/users]

### Retrieve all users [GET]

+ Response 200 (application/json)

  + Attributes (array[User])

## User [/users/{id}]

+ Parameters

    + id (required, number, `1`) ... The user ID

### Retrieve a User [GET]

+ Response 200 (application/json)

  + Attributes (User)

# Data Structures

## User (object)

+ id: 1 (number)
+ name: jules (string) - full name

If I break my app's /user/{id} endpoint and make it return e.g. a string rather than a number for the id attribute, dredd catches the error (which is awesome!).

Unfortunately this doesn't work for the array case: if I break my /users endpoint in any way but still return an array at the top level dredd doesn't catch the problem.

cilf commented 9 years ago

Hey guys, I'm having same issue as @jolleon. Do you have ETA on this getting fixed?

honzajavorek commented 8 years ago

@cilf @jolleon I think your issue was not related to the original one. When using Attributes, the API Blueprint parser generates the JSON Schema asset for you and Dredd uses that asset. There were some bugs in the parser since your comments and some of them are already fixed. If you still experience the problems, file another issue please.

If one doesn't use Attributes, but writes the JSON asset manually in API Blueprint but doesn't specify JSON Schema manually, then Gavel.js generates a simple JSON Schema for the JSON asset and uses it for validation. And that's what the original issue is about.

cilf commented 8 years ago

Hi! Thanks for looking into it. I just tested the behaviour with latest Dredd@1.0.7 and unfortunately it still ignores errors in objects in arrays.

I'd rather keep this issue since to me it looks like it's still the same problem. And the title says it all :-)

honzajavorek commented 8 years ago

@cilf Thanks for verifying!

I'll keep an eye on this. I still think that internally those are two separate issues and if I'm able to distinguish them myself by example, tests or inspecting code, I'll probably separate them, but until then I'll leave it as it is.

Thanks again for getting back to this and testing the thing with the newest Dredd version!

jmachan-zz commented 8 years ago

Hi guys! We updated Dredd on: User-Agent: Dredd/1.0.9 (Linux 3.13.0-24-generic; x64) and it still ignores errors in objects in array :(

w-vi commented 8 years ago

This is not really a bug but a feature I'm afraid. The issue is that in MSON everything is optional by default and therefore the JSON Schema generated cannot enforce the type in the array as there could be something else as well. The main issue is that in MSON things MAY but in JSON Schema they MUST ... see the discussion in MSON repo https://github.com/apiaryio/mson/issues/31

beyourselfman commented 8 years ago

@w-vi I have trouble to follow your MAY-MUST argumentation. In my opinion the distinction in json schema between may and must is made by the required field. In the following example:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "type": "array",
  "items": {
    "type": "object",
    "properties": {
      "name": {
        "type": "string"
      },
      "fields": {
        "type": "object",
        "properties": {
          "field-a": {
            "type": "string"
          },
          "field-b": {
            "type": "string"
          }
        },
        "required": [
          "field-a"
        ]
      }
    },
    "required": [
      "name",
      "fields"
    ]
  }
}

This response should be valid

[
  {
    "name": "name",
    "fields": {
      "field-a": "value-a"      
    }
  }
]

This response should be not valid (error: Invalid type. Expected String but got Integer.)

[
  {
    "name": "name",
    "fields": {
      "field-a": 1    
    }
  }
]

What am I missing here? ;-)

honzajavorek commented 8 years ago

@beyourselfman Both JSON Schema and MSON take by default some things as required, others as optional. In both there are means to go out of the default behavior (i.e. required, optional, additionalProperties, fixed, ...). However, mostly, the default behavior in JSON Schema is to strictly validate given structure, whereas the default behavior MSON is usually to just describe what may appear in given structure. This is why MSON lacks some means to behave more strictly. One of these means is to strictly limit contents of arrays.


Please, move the discussion to https://github.com/apiaryio/mson/issues/31, since this isn't a place where the issue can be resolved, neither it's a place where other people interested in the matter would find your notes and opinions.

honzajavorek commented 8 years ago

I'm going to split the issue, since it's getting polluted from an unrelated problem. The original issue here is about Gavel.js. What people now report here has nothing to do with Gavel.js.

If you use the Attributes keyword and array member objects won't get validated, you are affected by https://github.com/apiaryio/dredd/issues/449. It's feature/bug of MSON discussed at https://github.com/apiaryio/mson/issues/31. If you use raw payloads instead, you may be affected by bug in Gavel.js described and reported in first posts of this issue and feel free to subscribe here to be notified when it's fixed.

honzajavorek commented 7 years ago

@netmilk @ecordell Gavel.js generates correct JSON Schema (works on http://www.jsonschemavalidator.net/). E.g. having JSON body

+ Response 200 (application/json)

        [{"sub": 1}]

Gavel.js generates:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "id": "#",
  "additionalItems": true,
  "type": "array",
  "items": [
    {
      "id": "0",
      "additionalProperties": true,
      "type": "object",
      "required": [
        "sub"
      ],
      "properties": {
        "sub": {
          "id": "sub"
        }
      }
    }
  ]
}

That schema lets [{"sub": 1}] pass, but won't let [{"woohoo": 1}] to pass.

But Gavel.js provides true with gavel.isValid even if it shouldn't pass, so Dredd considers the test as passed. Classifying as a bug. I filed https://github.com/apiaryio/gavel.js/issues/91 as well.

honzajavorek commented 6 years ago

This is probably related: https://stackoverflow.com/questions/45568397/dredd-apiary-mson-array-of-any-size/45591800?noredirect=1#comment78147949_45591800

artem-zakharchenko commented 4 years ago

With the gavel@8.2.0 it should be possible to use root-level inferred schema based on the given array body. You can see the test suite for the spec and usage example.

This needs verification and can be closed, if confirmed working as expected.