Added support for JSON query parameters #43
Conversation
There was a problem hiding this comment.
Great work @samuelnogueira ! Some minor questions addressed
| $parameter = $specs[$name]; | ||
| $schema = $parameter->schema; | ||
| if ($schema === null) { | ||
| // There should be one and only one entry in 'content'. |
There was a problem hiding this comment.
It sounds that we rely on some kind of docs here. Can you share a link to it? What happens if we have zero entries? two entries?
There was a problem hiding this comment.
It sounds that we rely on some kind of docs here. Can you share a link to it?
I found strict rules on how to use schema/content in Swagger Editor. By the magic of testing all possible combinations, I found these are considered structural errors:
- Having both
schemaandcontent - Not having
schemaorcontent - Having
contentwith no properties - Having
contentwith more than 1 properties
The documentation in Swagger website is a bit vague here, I could only find this relevant quote:
https://swagger.io/docs/specification/describing-parameters/#schema-vs-content
To describe the parameter contents, you can use either the schema or content keyword. They are mutually exclusive (...)
It looks good enough to me, so I'll add a link to it in the comments.,
What happens if we have zero entries? two entries?
I think we should throw an \League\OpenAPIValidation\Schema\Exception\InvalidSchema for these cases. Will change the implementation accordingly, please let me know afterwards if you agree @scaytrase .
There was a problem hiding this comment.
Yea, I've checked editor too, we should throw invalid schema for count !== 1
| if (preg_match('#^application/.*json$#', $contentType)) { | ||
| $value = json_decode($value, true); | ||
| if (json_last_error() !== JSON_ERROR_NONE) { | ||
| throw InvalidQueryArgs::becauseValueIsNotValidJson(json_last_error_msg(), $argumentName, $addr); |
There was a problem hiding this comment.
How about more generic becauseDoesNotMatchContentType or something like this? We can reuse it later for XML or other types
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace League\OpenAPIValidation\Tests\FromCommunity; |
There was a problem hiding this comment.
I hope "FromCommunity" namespace exist to pin reproducing cases from different issues discussions into test flow. Since this is a general feature implementation, I suggest you to relocate this code to namespace related to new featuers
with the intent of having less code changes to add deserialization support for cookies, headers and path params. + Ambiguous/invalid content/schema specification now throws InvalidSchema exception
| return $value; | ||
| } | ||
|
|
||
| private function isJsonContentType() : bool |
There was a problem hiding this comment.
As a future improvement (not required for this PR) we can implement typed extensions for this class, like JsonRequestParameter, PlainRequestParameter instead of passing content-type and checking it on each deserialization
|
Sorry for late review, great work @samuelnogueira ! |
|
@scaytrase it's holiday season, the review wasn't late at all! Thank you for taking the time to review and merge the PR, it's much appreciated. |
This is part of the effort required for #2
Only adds support for JSON serialized query parameters, no support for other forms of serialization (ex. application/xml) were added, or other types of parameters (ex. cookies).
A use case for this type of parameters is used as example in OpenAPI documentation:
https://swagger.io/docs/specification/describing-parameters/#schema-vs-content