byjg / php-swagger-test

A set of tools for testing your REST calls based on the swagger documentation using PHPUnit
MIT License
98 stars 32 forks source link

Match is failing, when property is required and has null value #6

Closed makedo closed 6 years ago

makedo commented 6 years ago

In case, if property of definition is in required list, and in response that value exists in response, but equals null, match throws exception "Required property not found". Example:

     /* @SWG\Get(
     *   path="/v1/get",
     *   @SWG\Response(
     *    response=200
     *    @SWG\Schema(
     *      required={"data"}
     *     @SWG\Property(property="data",  type="string"),
     *    ),
     *   )
     * )
*/

Match against response: {"data": null} will throw an exception.

Proposed solution: SwaggerBody.php on line 116 replace isset with array_key_exists.

byjg commented 6 years ago

Hey, thank you!

I created a branch "issue-6" with your suggested change and a test case for it.

However, the question here is conceptual. Is the "Required Field" means that a field exists with any value or it is a field that have a non-empty value?

byjg commented 6 years ago

I read the OpenApi specification by my initial question is not answered.

References:

makedo commented 6 years ago

Hey, thank you for a quick response! What about nulls, swagger documentation says, that it not supported nulls in 2.0, and in 3.0 they added parameter "nullable" https://swagger.io/docs/specification/data-models/data-types/#null

makedo commented 6 years ago

But, as for me, "required" describes existing of a property in response, not it's value, so it does not matter if that value is null, or array or of some other type.

byjg commented 6 years ago

Yeah. You're right.

Here is the answer: https://stackoverflow.com/questions/45575493/what-does-required-in-openapi-really-mean

The "required" clause is not satisfied by this:

{"data": null}

There are a "nullable" property on the newest "OpenApi" specification but it is not implemented yet in Swagger-PHP yet.

But at this moment it is not implemented yet nor in the Pet example nor in the PHP Swagger. Any way we need to create this "nullable " argument. I create some test cases in the issue's branch, but I can only dig into this at weekend. Can you help me with this implementation?

makedo commented 6 years ago

Sure, I'll try to start implementing that today in the evening, and try to prepare some solution or partial solution till this weekend. Thank you!

makedo commented 6 years ago

So, what about adding that nullable parameter, I think there's no sense to do it in current version, because current version is for OpenApi2.0 specification. I tried to generate swagger.json on branch 3.x of swagger-php, and it has a lot of deprecations, f.e. Annotation Definition is deprecated, and format of json file is also changed, so I think the better way to solve that problem - start working on hooking up json file of OpenApi 3.0 format, because adding nullable paramater to annotation and trying generating json file on swagger-php for 2.0 caused an exception.

And what about solving that in current 2.0 implementation, because there is no info about nulls in 2.0 standart, what do you think of adding parameter allowNulls or smth to constructor of SwaggerSchema class, false by default, and than basing on that value throw or not throw exception in case of null? Thanks!

byjg commented 6 years ago

That's make sense. Let's do what you suggest. You can go on.

makedo commented 6 years ago

Prepared PR https://github.com/byjg/php-swagger-test/pull/7

byjg commented 6 years ago

I received your PR and I am reviewing.

byjg commented 6 years ago

Hi,

I published your PR and assigned a new version: 1.2.0

Thank you very much for your help here :)