Skip to content

[BUG] PHP Client - ObjectSerializer::buildQuery flattens array params resulting invalid URL params (param=a&param=b vs param[]=a&param[]=b) #19233 #19236

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

Merged
merged 3 commits into from
Aug 7, 2024
Merged

[BUG] PHP Client - ObjectSerializer::buildQuery flattens array params resulting invalid URL params (param=a&param=b vs param[]=a&param[]=b) #19233 #19236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37140b2
[BUG] PHP Client - ObjectSerializer::buildQuery flattens array params…
serbanghita Jul 24, 2024
a991d06
Added tests (replaced old provider data). This looks like a breaking …
Jul 26, 2024
59a8412
Fix space
Jul 29, 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
5 changes: 5 additions & 0 deletions modules/openapi-generator/src/main/resources/php-nextgen/ObjectSerializer.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,11 @@ class ObjectSerializer

$value = $flattenArray($value, $paramName);

// https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#style-values
if ($openApiType === 'array' && $style === 'deepObject' && $explode) {
return $value;
}

if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) {
return $value;
}
Expand Down
7 changes: 6 additions & 1 deletion modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,11 @@ class ObjectSerializer

$value = $flattenArray($value, $paramName);

// https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#style-values
if ($openApiType === 'array' && $style === 'deepObject' && $explode) {
return $value;
}

if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) {
return $value;
}
Expand Down Expand Up @@ -463,7 +468,7 @@ class ObjectSerializer
// determine file name
if (
is_array($httpHeaders)
&& array_key_exists('Content-Disposition', $httpHeaders)
&& array_key_exists('Content-Disposition', $httpHeaders)
&& preg_match('/inline; filename=[\'"]?([^\'"\s]+)[\'"]?$/i', $httpHeaders['Content-Disposition'], $match)
) {
$filename = Configuration::getDefaultConfiguration()->getTempFolderPath() . DIRECTORY_SEPARATOR . self::sanitizeFilename($match[1]);
Expand Down
5 changes: 5 additions & 0 deletions samples/client/echo_api/php-nextgen/src/ObjectSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,11 @@ public static function toQueryValue(

$value = $flattenArray($value, $paramName);

// https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#style-values
if ($openApiType === 'array' && $style === 'deepObject' && $explode) {
return $value;
}

if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) {
return $value;
}
Expand Down
5 changes: 5 additions & 0 deletions samples/client/petstore/php-nextgen/OpenAPIClient-php/src/ObjectSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,11 @@ public static function toQueryValue(

$value = $flattenArray($value, $paramName);

// https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#style-values
if ($openApiType === 'array' && $style === 'deepObject' && $explode) {
return $value;
}

if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) {
return $value;
}
Expand Down
7 changes: 6 additions & 1 deletion samples/client/petstore/php/OpenAPIClient-php/lib/ObjectSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,11 @@ public static function toQueryValue(

$value = $flattenArray($value, $paramName);

// https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#style-values
if ($openApiType === 'array' && $style === 'deepObject' && $explode) {
return $value;
}

if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) {
return $value;
}
Expand Down Expand Up @@ -472,7 +477,7 @@ public static function deserialize($data, $class, $httpHeaders = null)
// determine file name
if (
is_array($httpHeaders)
&& array_key_exists('Content-Disposition', $httpHeaders)
&& array_key_exists('Content-Disposition', $httpHeaders)
&& preg_match('/inline; filename=[\'"]?([^\'"\s]+)[\'"]?$/i', $httpHeaders['Content-Disposition'], $match)
) {
$filename = Configuration::getDefaultConfiguration()->getTempFolderPath() . DIRECTORY_SEPARATOR . self::sanitizeFilename($match[1]);
Expand Down
14 changes: 8 additions & 6 deletions samples/client/petstore/php/OpenAPIClient-php/tests/ObjectSerializerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public function testSanitizeFilename(): void
$this->assertSame("sun.gif", ObjectSerializer::sanitizeFilename("../sun.gif"));
$this->assertSame("sun.gif", ObjectSerializer::sanitizeFilename("/var/tmp/sun.gif"));
$this->assertSame("sun.gif", ObjectSerializer::sanitizeFilename("./sun.gif"));

$this->assertSame("sun", ObjectSerializer::sanitizeFilename("sun"));
$this->assertSame("sun.gif", ObjectSerializer::sanitizeFilename("..\sun.gif"));
$this->assertSame("sun.gif", ObjectSerializer::sanitizeFilename("\var\tmp\sun.gif"));
Expand Down Expand Up @@ -62,7 +62,7 @@ public function testDeserializeFile($stream, ?array $httpHeaders = null, ?string
* File Streams Provider
* @return array[]
*/
public function provideFileStreams(): array
public static function provideFileStreams(): array
{
return [
'File stream without headers' => [
Expand Down Expand Up @@ -112,7 +112,7 @@ public function testDateTimeParseSecondAccuracy(string $timestamp, string $expec
*
* @return string[][]
*/
public function provideTimestamps(): array
public static function provideTimestamps(): array
{
return [
'String from #7942' => [
Expand Down Expand Up @@ -173,7 +173,7 @@ public function testToQueryValue(
*
* @return array[]
*/
public function provideQueryParams(): array
public static function provideQueryParams(): array
{
$array = ['blue', 'black', 'brown'];
$object = ['R' => 100, 'G' => 200, 'B' => 150];
Expand Down Expand Up @@ -300,10 +300,12 @@ public function provideQueryParams(): array
'deepObject array, explode off, required true' => [
$array, 'color', 'array', 'deepObject', false, true, 'color=blue%2Cblack%2Cbrown',
],
// color=blue&color=black&color=brown

// color[0]=blue&color[1]=black&color[2]=brown
'deepObject array, explode on, required true' => [
$array, 'color', 'array', 'deepObject', true, true, 'color=blue&color=black&color=brown',
$array, 'color', 'array', 'deepObject', true, true, 'color%5B0%5D=blue&color%5B1%5D=black&color%5B2%5D=brown',
Copy link
Member

Choose a reason for hiding this comment

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

according to https://swagger.io/docs/specification/serialization/, the behavior for an array with deepObject style and explode set to true should be undefined.

if you want to send an array in query parameter, should the style be something else instead of deepObject?

(i'm not against this change as it allows users to send array with index but just want to confirm that's what the server intends to receive from the PHP client)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 I presume you are referring to the paragraph:

deepObject – simple non-nested objects are serialized as paramName[prop1]=value1&paramName[prop2]=value2&.... The behavior for nested objects and arrays is undefined.

I believe the reson for "The behavior for nested objects and arrays is undefined." is explained in this comment:

According to OAI/OpenAPI-Specification#1006 (comment):

Tooling will have to deal with it, and I'd suggest just clobbering values. It's really up to the user to define things that make sense, within the defined behavior.
Authoring tools might be able to provide warnings to users in such a case.

My justification for this BEHAVING like that is #19233 (comment).

if you want to send an array in query parameter, should the style be something else instead of deepObject?

My justification for this BEING in deepObject is that my problem was in the query params as opposed to body of a form payload.
Because of the PHP behaviour of $_GET and $_POST I believe both form + deepObject with explode: true should serialize with brakets []

(i'm not against this change as it allows users to send array with index but just want to confirm that's what the server intends to receive from the PHP client)

If you code in PHP, the expectations are exactly like in my POC

PS: The way we mitigated this is by migrating the OpenAPI schema from get: to

post:
  requestBody:
  ...
   color:
      type: array
      items:
          type: string

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation.

let's give it a try

],

// color[R]=100&color[G]=200&color[B]=150
'deepObject object, explode off, required true' => [
$object, 'color', 'object', 'deepObject', false, true, 'color%5BR%5D=100&color%5BG%5D=200&color%5BB%5D=150',
Expand Down
7 changes: 6 additions & 1 deletion samples/client/petstore/php/psr-18/lib/ObjectSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,11 @@ public static function toQueryValue(

$value = $flattenArray($value, $paramName);

// https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#style-values
if ($openApiType === 'array' && $style === 'deepObject' && $explode) {
return $value;
}

if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) {
return $value;
}
Expand Down Expand Up @@ -472,7 +477,7 @@ public static function deserialize($data, $class, $httpHeaders = null)
// determine file name
if (
is_array($httpHeaders)
&& array_key_exists('Content-Disposition', $httpHeaders)
&& array_key_exists('Content-Disposition', $httpHeaders)
&& preg_match('/inline; filename=[\'"]?([^\'"\s]+)[\'"]?$/i', $httpHeaders['Content-Disposition'], $match)
) {
$filename = Configuration::getDefaultConfiguration()->getTempFolderPath() . DIRECTORY_SEPARATOR . self::sanitizeFilename($match[1]);
Expand Down
Loading