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.

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] Allows passing filename to deserialize #11582

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

jtreminio
Copy link
Contributor

@jtreminio jtreminio commented Feb 11, 2022

Currently the ObjectSerializer::deserialize() does not work when passing a filename string for a SplFileObject type.

The \GuzzleHttp\Psr7\Utils::streamFor() has support for a wide-range of data, including strings, and converts them to a stream resource.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jtreminio
Copy link
Contributor Author

Copy link
Contributor

@ybelenko ybelenko left a comment

Choose a reason for hiding this comment

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

Checked locally:

vendor/bin/phpunit tests/ObjectSerializerTest.php

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

....................                                              20 / 20 (100%)

Time: 00:00.040, Memory: 6.00 MB

OK (20 tests, 31 assertions)

There was some breaking changes in OpenApi 3 schema if I remember correctly. SplFileObject has been deprecated and replaced with binary, maybe it's not related, @wing328 will correct me.

Looks totally ok. Now I'm trying to imagine the use case... Hmm, deserialization, right? Then it should be response from remote server with already initiated \StreamInterface in response body. Maybe Guzzle can transcode response on the fly that way, but I don't know. Seems like it will never happen 🤣

@@ -346,6 +347,8 @@ public static function deserialize($data, $class, $httpHeaders = null)
}

if ($class === '\SplFileObject') {
$data = Psr7\Utils::streamFor($data);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use either full namespace \GuzzleHttp\Psr7\Utils::streamFor() or just Utils::for() with import. Your way looks like a half measure and a bit confusing. Imagine you're reading the code briefly then it's hard to say whether Psr7\Utils class is local or from vendor package.

Copy link
Contributor Author

@jtreminio jtreminio Feb 12, 2022

Choose a reason for hiding this comment

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

Looks totally ok. Now I'm trying to imagine the use case... Hmm, deserialization, right? Then it should be response from remote server with already initiated \StreamInterface in response body. Maybe Guzzle can transcode response on the fly that way, but I don't know. Seems like it will never happen 🤣

I am going at this from the opposite end, if a user wants to pass an array of data to have the SDK instantiate the full object (including nested objects) for them.

API responses wouldn't ever contain an SplFileObject object, but a user can call

require_once __DIR__ . "/vendor/autoload.php";

$config = OpenAPI\Client\Configuration::getDefaultConfiguration();
$config->setUsername("YOUR_API_KEY");

$api = new OpenAPI\Client\Api\FooRequestApi(
    new GuzzleHttp\Client(),
    $config
);

$data = [
    "file" => "/path/to/file.pdf",
    "prop1" => "",
    "prop2" => "",
    "prop3" => "",
    // ... other data here
];

$obj = ObjectSerializer::deserialize($data, ModelClass::class)

try {
    $result = $api->fooRequestSend($obj);
    print_r($result);
} catch (Exception $e) {
    echo "Exception when calling API: " . $e->getMessage() . PHP_EOL;
}

I believe this is a preferable alternative to having to do:

$obj = new ModelClass();
$obj->setFile(new SplFileObject("/path/to/file.pdf"))
    ->setProp1(/* ... */)
    ->setProp2(/* ... */)
    ->setProp3(/* ... */);

If a user has prebuilt JSON data they can simply pass the data to ObjectSerializer::deserialize() and everything works. Likewise with unit tests, now we can just have a bundle of pre-defined JSON data fixtures we can use to run many unit tests using data providers.

Copy link
Contributor

@ybelenko ybelenko Feb 12, 2022

Choose a reason for hiding this comment

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

Hmm, thanks for such a great explanation. That's confusing SDK behaviour. I thought about serialize and deserialize methods like encode and decode way. User shouldn't even know about these methods, SDK itself should prepare data before request. Unfortunately, we all understand current PHP codegen is far from perfect.

Regarding your example, I thought I can do the same with sanitizeForSerialization call 🤣. Just checked code of sanitizeForSerialization and it feels like I cannot achieve the same with that method, probably you right.

Copy link
Contributor Author

@jtreminio jtreminio Feb 12, 2022

Choose a reason for hiding this comment

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

@ybelenko I've got a related ticket opened at #11432

Because PHP constructor can only return an instance of itself, this does not work:

$obj = new ModelClass([
    "file" => "/path/to/file.pdf",
    "prop1" => "",
    "prop2" => "",
    "prop3" => "",
    // ... other data here
]);

All values are set as literal data, no transformation is done. I would argue this is undesired behavior. The user passes data to the class and would expect everything to just work. If they simply do the above and then immediately call the API method to send the data there is no problem.

However, if they try to do, assuming prop1 is an array of objects,

$obj = new ModelClass([
    "file" => "/path/to/file.pdf",
    "prop1" => "",
    "prop2" => "",
    "prop3" => "",
    // ... other data here
]);

foreach ($obj->getProp1() as $prop1) {
    // ...
}

This wouldn't work as expected if prop1 is supposed to be, say, Foo[]. The user would expect iterating over it via the getter would be the correct object but it would simply be the actual array data they originally sent.

I am working on a PR to allow something like

class ModelClass implements ModelInterface, ArrayAccess, JsonSerializable
{
    // ...

    public static function fromArray(array $data): ModelClass
    {
        $obj = ObjectSerializer::deserialize(
            $data,
            ModelClass::class,
        );

        return $obj;
    }

So the user can simply do

$obj = ModelClass::fromArray([
    "file" => "/path/to/file.pdf",
    "prop1" => "",
    "prop2" => "",
    "prop3" => "",
    // ... other data here
]);

and now they have a correctly instantiated object with the correct return type hinting.

@wing328 wing328 added this to the 6.0.0 milestone Feb 14, 2022
@wing328
Copy link
Member

wing328 commented Feb 14, 2022

All tests passed:

PHPUnit 9.5.11 by Sebastian Bergmann and contributors.

..............* Closing connection 0
.* Closing connection 0
.................................................. 65 / 92 ( 70%)
...........................                                       92 / 92 (100%)

Time: 00:01.784, Memory: 12.00 MB

OK (92 tests, 242 assertions)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  4.106 s
[INFO] Finished at: 2022-02-14T17:37:51+08:00
[INFO] ------------------------------------------------------------------------

@wing328 wing328 merged commit 905e59c into OpenAPITools:master Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants