Open SergeyNovikovDH opened 7 years ago
@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 Option
al 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?
@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?
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.
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.
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.
👍
Endpoint:
definition:
request body:
Expected result: validation error (
language
property is required) Actual result: no errors