apiaryio / dredd

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

Enums can't be nullable #507

Open honzajavorek opened 8 years ago

honzajavorek commented 8 years ago

Problems with nullable enums were reported by multiple users in https://github.com/apiaryio/mson/issues/61. We should investigate the problem.

pete001 commented 8 years ago

We are running into the same issue, would be great to get this fixed.

matthewdias commented 7 years ago

Any updates on this?

honzajavorek commented 7 years ago

Not yet, thanks for bumping this. I didn't have time to investigate exactly what's the root cause of the problem. Since in https://github.com/apiaryio/mson/issues/61 they report it should not be an issue in the parser, my first guess would be that dredd-transactions do not transform API Elements to transactions correctly. Any help or at least failing tests appreciated.

JeppeKnockaert commented 7 years ago

I've got an example reproducing this issue. I believe it is the generated JSON schema that is incorrect.

My apib file:

FORMAT: 1A

# Test API

### Test route [GET /test]

+ Response 200 (application/json)
    + Attributes (Item, fixed-type)

# Data Structures

## Item (object)

- type (Type, nullable)

## Type (enum)

- type1
- type2

Generated schema versus the tested response: https://jsonschemalint.com/#/version/draft-04/markup/json?gist=86082c8c39b53359fe6835163fc65569

If I add null (without quotes) as one of the possible enum types, the generated JSON schema includes "null" (with quotes) as a possible enum type.

So, this:

## Type (enum)

- null
- type1
- type2

results in this generate schema versus the tested response: https://jsonschemalint.com/#/version/draft-04/markup/json?gist=f80347a08900d60c4d264b9c8bd1ca79

Finally, I'd like to conclude with a working combination: https://jsonschemalint.com/#/version/draft-04/markup/json?gist=458a3729bd732bd5109dbe1fc5d93619

honzajavorek commented 7 years ago

@JeppeKnockaert Awesome! Thanks for the investigation and for filing https://github.com/apiaryio/drafter/issues/455!

honzajavorek commented 7 years ago

This was fixed in Drafter. Soon it should propagate to Dredd.

kylef commented 6 years ago

The fix from Drafter should be in Dredd by now, is there any remaining problems? Perhaps this issue can be closed.

honzajavorek commented 6 years ago

As your "QA guy" (quoting @pksunkara) I'd like to have this (and this https://github.com/apiaryio/dredd/issues/283#issuecomment-385720973) tested, at least superficially, in Dredd.

octalmage commented 6 years ago

I think I'm still having an issue with this and swagger:

      environment:
        type: string
        x-nullable: true
        enum:
        - production
        - staging
        - development

I get:

fail: body: At '/results/3/environment' Invalid type: null (expected string)

But if I add null to the enum, it works. Both x-nullable and null in the enum have to exist though.

honzajavorek commented 6 years ago

@apiaryio/adt is the behavior described by @octalmage expected?

pksunkara commented 6 years ago

Yes, x-nullable by default would mean null in enum. But when you provide a different enum, then they would contradict and enum gets priority since it is actually directly JSON Schema instead of an extension.

kylef commented 6 years ago

The x-nullable flag only adds null to the type of the JSON Schema. The problem here is when enum is present in JSON Schema, validators don't look at type since the type information isn't needed when you have typed-values. I think it makes sense for nullable to add null as a value to the enum when nullable is set and there is also enum. This logic should be added to fury-adapter-swagger. The specific code for this logic is found at https://github.com/apiaryio/fury-adapter-swagger/blob/46af39bbdd88b5a40910fb7d016e792e77299752/src/json-schema.js#L13-L17 and the tests at https://github.com/apiaryio/fury-adapter-swagger/blob/46af39bbdd88b5a40910fb7d016e792e77299752/test/json-schema.js#L53-L71 if anyone is interested in contributing this change as a PR. We should hopefully schedule this soon into one of our engineering sprints, but external contributions are always welcome.

andybarilla commented 5 years ago

I'm still having this issue whether I'm including null in the enum list or not.

Swagger example:

swagger: '2.0'
info:
  version: 1.0.0
  title: nullable enum
paths:
  /test:
    get:
      responses:
        200:
          description: OK
          schema:
            $ref: '#/definitions/NullableEnum'
        201:
          description: Created
          schema:
            $ref: '#/definitions/NullableEnumPlus'
definitions:
  NullableEnum:
    type: object
    additionalProperties: false
    properties:
      color:
        type: string
        x-nullable: true
        enum:
          - red
          - blue
          - green
  NullableEnumPlus:
    type: object
    additionalProperties: false
    properties:
      color:
        type: string
        x-nullable: true
        enum:
          - red
          - blue
          - green
          - null

Testing:

# npx dredd nullable-enum.yaml https://localhost --dry-run
error: Parser error in file 'nullable-enum.yaml': Cannot read property '$ref' of null on line 11
error: Parser error in file 'nullable-enum.yaml': Cannot read property '$ref' of null on line 15
error: Error when processing API description.
kylef commented 5 years ago

@andybarilla I've prepared a fix in https://github.com/apiaryio/api-elements.js/pull/59, I hope you don't mind that I've taken parts of your example Swagger 2 document to reproduce in our test fixtures.

honzajavorek commented 4 years ago

This is done, the only thing missing is an integration or e2e test in Dredd verifying this works as intended.

kylef commented 4 years ago

It would seem that in OpenAPI 3, the exact opposite to my prior comment on making nullable add null to enums,. https://github.com/OAI/OpenAPI-Specification/pull/2050 where the proposal from TSC members to clarify the wording of nullable to become:

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

Thus, nullable would only apply to the type, nullable keyword is a modifier of the type keyword. In particular the question at https://github.com/OAI/OpenAPI-Specification/blob/7d94b7c4ac0650318887d8f9588af50c7318a698/proposals/003_Clarify-Nullable.md#questions-answered

If a schema specifies nullable: true and enum: [1, 2, 3], does that schema allow null values?

No. The nullable: true assertion folds into the type assertion, which presumably specifies integer or number.

dpashkevich commented 4 years ago

Note that the nullable attribute is likely to go away in OAS 3.1

# OAS 3.0
type: string
nullable: true

# OAS 3.1 (unreleased)
type:
  - string
  - 'null'

See https://github.com/OAI/OpenAPI-Specification/pull/2246