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

[8.x] Add ability to customize json options on JsonResource response #40208

Merged
merged 3 commits into from
Jan 3, 2022
Merged

[8.x] Add ability to customize json options on JsonResource response #40208

merged 3 commits into from
Jan 3, 2022

Conversation

bastien-phi
Copy link
Contributor

Currently, it is possible to customize the options with JsonResource::withResponse($request, $response) doing something like

public function withResponse($request, $response)
{
    $response->setEncodingOptions(JSON_PRETTY_PRINT);
}

The behaviour is that the original json is decoded then re-encoded with the given options

public function setEncodingOptions($options)
{
$this->encodingOptions = (int) $options;
return $this->setData($this->getData());
}

public function getData($assoc = false, $depth = 512)
{
return json_decode($this->data, $assoc, $depth);
}

public function setData($data = [])
{
$this->original = $data;
if ($data instanceof Jsonable) {
$this->data = $data->toJson($this->encodingOptions);
} elseif ($data instanceof JsonSerializable) {
$this->data = json_encode($data->jsonSerialize(), $this->encodingOptions);
} elseif ($data instanceof Arrayable) {
$this->data = json_encode($data->toArray(), $this->encodingOptions);
} else {
$this->data = json_encode($data, $this->encodingOptions);
}
if (! $this->hasValidJson(json_last_error())) {
throw new InvalidArgumentException(json_last_error_msg());
}
return $this->update();
}

Unfortunately, json_encode with no option is destructive :
json_decode(json_encode(['float' => 3.0]), true) gives ['float' => 3].

With this, $response->setEncodingOptions(JSON_PRESERVE_ZERO_FRACTION); returns a json like

{"data":{"float":3}}

That is, there is currently no way to encode JsonResources with JSON_PRESERVE_ZERO_FRACTION.

This PR adds the ability to customize json serialization options for responses from JsonResource at the time that the JsonResponse is created, providing the ability to preserve the zero fraction of floats.

Why do you need JSON_PRESERVE_ZERO_FRACTION btw ?

Imagine a Order model with a quantity property as float and you want to test some simple api call like

public function it_returns_the_order() 
{
    $order = Order::factory()->createOne();
    
    $this->getJson("api/orders/{$order->id}")
        ->assertOk()
        ->assertJsonPath('data', [
            'id' => $order->id,
            'quantity' => $order->quantity,
        ]);
}

Pretty easy no ?

Well, if your factory is defined with

public function definition()
{
    return [
        'quantity' => $this->faker->randomFloat(1, 0, 10), // random float with 1 digit max between 0 and 10
    ];
}

and faker generates a float with 1 digit, your test will pass. But if faker generates 3.0 for example, the test will fail with

Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-  'quantity' => 0.0
+  'quantity' => 0

Quite annoying to have randomly failing tests...

@taylorotwell
Copy link
Member

What about resource collections? Does anything need to be done for them?

@bastien-phi
Copy link
Contributor Author

No, no change needed for ResourceCollection.

But you make me remember that the same change must be done in PaginatedResourceResponse
https://github.com/laravel/framework/blob/8.x/src/Illuminate/Http/Resources/Json/PaginatedResourceResponse.php#L15

Currently afk, I will make the change tomorrow.

@bastien-phi bastien-phi marked this pull request as draft December 30, 2021 22:47
@bastien-phi
Copy link
Contributor Author

I will reopen the PR once the changes are pushed.

@bastien-phi
Copy link
Contributor Author

Well, you were right, some changes were needed for ResourceCollections... Let me know if something looks wrong

@bastien-phi bastien-phi marked this pull request as ready for review December 31, 2021 07:43
@otilor
Copy link

otilor commented Dec 31, 2021

Really good work @bastien-phi

@taylorotwell taylorotwell merged commit 53a9850 into laravel:8.x Jan 3, 2022
@bastien-phi
Copy link
Contributor Author

Thanks !

@bastien-phi bastien-phi deleted the add_json_options_to_json_resource branch January 3, 2022 20:00
@biila-matti
Copy link

biila-matti commented Feb 2, 2022

This has introduced an error for me:

namespace App\Http\Resources;

use App\Models\User;
use Illuminate\Http\Resources\Json\ResourceCollection;

class UserResourceCollection extends ResourceCollection
{
    public $collects = User::class;

    /**
     * Transform the resource collection into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request)
    {
        return $this->collection->map(function ($user) use ($request) {
            // ...
        });
    }

    // ...
}

Using this ResourceCollection throws BadMethodCallException with message 'Call to undefined method App\Models\User::jsonOptions()'.

Should I not use model classes in $collects property anymore? In this resource collection I do not want it to make each row into a UserResource instance.

I've been following this guide to make it work the way I want and it has worked just fine before.

@driesvints
Copy link
Member

@bastien-phi can you fix the issue from above? Otherwise it'll be best to revert this PR.

@bastien-phi
Copy link
Contributor Author

@driesvints It looks like an unexpected usage of ResourceCollection. As documentation states, $collects should be a class-string of JsonResource
image

As it worked with that, I will open a PR to fix this.

@driesvints
Copy link
Member

@bastien-phi you're correct. @ninjami-matti it seems you're using a model here but $collects is intended for resources only. That's also explained in the guide you're linking to.

@biila-matti
Copy link

biila-matti commented Feb 4, 2022

Fair enough 👍

I would still argue that it is slightly confusing that you can still make ResourceCollection classes without a corresponding JsonResource class existing, which then makes the ResourceCollection collect the actual Model class as the resource.

I set the $collects property with a Model class based on the assumption that it should work since using the Model does work when not explicitly defined. And it did work before this addition.

But indeed I see now that setting a Model class to the $collects property is the wrong way to go about this. Thanks for the help!

@bastien-phi
Copy link
Contributor Author

@ninjami-matti If you want to bypass auto-discovery of underlying resource, you can just rename your resource collection in order to make it not ending with Collection and remove $collects

class UserCollectionResource extends ResourceCollection
{
    /**
     * Transform the resource collection into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request)
    {
        return $this->collection->map(function (\App\Models\User $user) use ($request) {
            // ...
        });
    }

    // ...
}

will work

@biila-matti
Copy link

That's true! However, I'm just too much of a stickler on following "correct" naming conventions 😄 So I went ahead and changed my collections and resources to reflect the correct usage and the problem is now gone.

Thanks for the tip!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants