Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PHP-Client] Allow Content-Type merge-match+json for encoding #19479

Merged
merged 6 commits into from
Oct 3, 2024
Merged

[PHP-Client] Allow Content-Type merge-match+json for encoding #19479

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69c1434
Allow Content-Type merge-match+json for encoding
frickelbruder Aug 28, 2024
c0eb373
Changes JSON recognition to more flexible regex
frickelbruder Sep 4, 2024
11a483e
Removes now useless JSON format list
frickelbruder Sep 4, 2024
40de0a4
Removes empty spaces
frickelbruder Sep 9, 2024
30f6f05
Adds explanatory PHPDoc comment
frickelbruder Sep 9, 2024
d05fac4
Moves Json-detection to class HeaderSelector
frickelbruder Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions modules/openapi-generator/src/main/resources/php/libraries/psr-18/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ use function sprintf;
* @var StreamFactoryInterface
*/
protected $streamFactory;

frickelbruder marked this conversation as resolved.
Show resolved Hide resolved
public function __construct(
ClientInterface $httpClient = null,
Configuration $config = null,
Expand Down Expand Up @@ -613,7 +613,7 @@ use function sprintf;
// for model (json/xml)
{{#bodyParams}}
if (isset(${{paramName}})) {
if ($headers['Content-Type'] === 'application/json') {
if ($this->bodyShouldBeEncoded($headers['Content-Type'])) {
$httpBody = json_encode(ObjectSerializer::sanitizeForSerialization(${{paramName}}));
} else {
$httpBody = ${{paramName}};
Expand All @@ -637,7 +637,7 @@ use function sprintf;
// for HTTP post (form)
$httpBody = new MultipartStream($multipartContents);

} elseif ($headers['Content-Type'] === 'application/json') {
} elseif ($this->bodyShouldBeEncoded($headers['Content-Type'])) {
$httpBody = json_encode($formParams);

} else {
Expand Down Expand Up @@ -769,5 +769,10 @@ use function sprintf;

return $uri;
}

private function bodyShouldBeEncoded(string $contentType): bool
{
return preg_match('~^application/(json|[\w!#$&.+-^_]+\+json)\s*(;|$)~', $contentType) === 1;
}
}
{{/operations}}
11 changes: 8 additions & 3 deletions samples/client/petstore/php/psr-18/lib/Api/AnotherFakeApi.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class AnotherFakeApi
* @var StreamFactoryInterface
*/
protected $streamFactory;

public function __construct(
ClientInterface $httpClient = null,
Configuration $config = null,
Expand Down Expand Up @@ -376,7 +376,7 @@ public function call123TestSpecialTagsRequest($client)

// for model (json/xml)
if (isset($client)) {
if ($headers['Content-Type'] === 'application/json') {
if ($this->bodyShouldBeEncoded($headers['Content-Type'])) {
$httpBody = json_encode(ObjectSerializer::sanitizeForSerialization($client));
} else {
$httpBody = $client;
Expand All @@ -396,7 +396,7 @@ public function call123TestSpecialTagsRequest($client)
// for HTTP post (form)
$httpBody = new MultipartStream($multipartContents);

} elseif ($headers['Content-Type'] === 'application/json') {
} elseif ($this->bodyShouldBeEncoded($headers['Content-Type'])) {
$httpBody = json_encode($formParams);

} else {
Expand Down Expand Up @@ -489,4 +489,9 @@ private function createUri(

return $uri;
}

private function bodyShouldBeEncoded(string $contentType): bool
{
return preg_match('~^application/(json|[\w!#$&.+-^_]+\+json)\s*(;|$)~', $contentType) === 1;
}
}
9 changes: 7 additions & 2 deletions samples/client/petstore/php/psr-18/lib/Api/DefaultApi.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class DefaultApi
* @var StreamFactoryInterface
*/
protected $streamFactory;

public function __construct(
ClientInterface $httpClient = null,
Configuration $config = null,
Expand Down Expand Up @@ -371,7 +371,7 @@ public function fooGetRequest()
// for HTTP post (form)
$httpBody = new MultipartStream($multipartContents);

} elseif ($headers['Content-Type'] === 'application/json') {
} elseif ($this->bodyShouldBeEncoded($headers['Content-Type'])) {
$httpBody = json_encode($formParams);

} else {
Expand Down Expand Up @@ -464,4 +464,9 @@ private function createUri(

return $uri;
}

private function bodyShouldBeEncoded(string $contentType): bool
{
return preg_match('~^application/(json|[\w!#$&.+-^_]+\+json)\s*(;|$)~', $contentType) === 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding this function in the api client class instead?

can we name this method as isJsonMime?

minor suggestion: add a docstring explaining what this function does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, what you mean by "what about adding this function in the api client class instead?"
As this is a method used throughout the code and if not working properly results in Exceptions being thrown, I think it is quite important.
What I could think of, is implementing a separate Class "JsonDetector" or anything with a single method, which could be injected via consturctor DI. Would this be a good solution or are you thinking of anything else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the confusion, api client class (ApiClient.php) is in client generated by php-dt generator but not php client generator.

shall we put the function in HeaderSelector class instead which has something similar already?

openapi-generator/samples/client/petstore/php/psr-18/lib/HeaderSelector.php

Line 88 in 11a483e

$headersWithJson = preg_grep('~(?i)^(application/json|[^;/ \t]+/[^;/ \t]+[+]json)[ \t]*(;.*)?$~', $accept);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you have something like this in mind?

}
Loading