Required properties validation about api-first-hand HOT 5 OPEN

@s12v, sorry for looking so late into it.

I have two remarks.

First, we don't have a null validation because we model it with Optional types. That is, anything which is not required is wrapped into an Option and the user has to deal with that. That is exactly the reason why this https://github.com/zalando/api-first-hand/pull/78/files#diff-c62960d334c61f68da8bb96a34038420R34 looks so alien.

Second, I can't really reproduce the issue. This is a specification I'm using:

swagger: '2.0'
info:
  version: 1.0
  title: Swagger Example
  description: bla
basePath: /example
schemes:
  - http
consumes:
  - application/json
produces:
  - application/json
paths:
  /token:
    post:
      parameters:
        - name: query
          in: body
          required: true
          schema:
            "$ref": "#/definitions/Query"
      responses:
        200:
          description: bla
          schema:
            $ref: "#/definitions/Query"
definitions:
  Query:
    type: object
    required:
      - language
    properties:
      language:
        type: string

And these are tests:

curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{}' 'http://localhost:9000/example/token'

JsResultException: JsResultException(errors:List((/language,List(ValidationError(List(error.path.missing),WrappedArray())))))


curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d 'http://localhost:9000/example/token'

Bad Request: Invalid Json: No content to map due to end-of-input\n at [Source: akka.util.ByteIterator$ByteArrayIterator$$anon$1@7c955531; line: 1, column: 0]


curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{"language": null }' 'http://localhost:9000/example/token'

JsResultException: JsResultException(errors:List((/language,List(ValidationError(List(error.expected.jsstring),WrappedArray())))))


curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{"language": "brainfuck" }' 'http://localhost:9000/example/token'

{"language":"brainfuck"}

I guess the error messages do not respect Accept header but for me it looks like the non-optionality is working. If it's not, I'd propose to investigate how to get it to work with the Option type, not with the validation.

Does it make sense?

from api-first-hand.

@slavaschmidt, thanks for looking into it. Behavior has changed in 0.2.0. Before, such errors were ignored and language property was null. Now it throws play.api.libs.json.JsResultException, which is still not optimal. Would be better to have error 400 with a corresponding message, IMO.

For missing query parameters it returns "Missing parameter: test1" and error 400 - not the standard validation error, but I think it's ok.

Missing path parameter it's 404 - looks good.

Regarding Option - I don't understand, required properties are not optionals. My PR handles only required properties.

I think I change the code to work only with Schema, it will be shorter and tests will pass ;-)

WDYT?

from api-first-hand.

Hm, actually my fix not going to work anymore, because JsResultException is thrown before validation (makes sense). Any idea how to handle it? Right now it's error 500 - internal server error. Should be 400 + message.

from api-first-hand.

Regarding Option - I don't understand, required properties are not optionals. My PR handles only required properties.

Exactly! Everything which is required is by definition not allowed to be null without additional validations. Everything not required is defined as an Option[T] with null values represented as None.

500 is bad, let us do something with that.

from api-first-hand.

Exactly! Everything which is required is by definition not allowed to be null without additional validations

I see. You're right. But it wasn't like this before! :) Looks like marshalling has been changed in 0.2.0

500 is bad, let us do something with that.

👍

from api-first-hand.