MatanYadaev / laravel-eloquent-spatial

Laravel Eloquent spatial package.
MIT License
315 stars 48 forks source link

Unable to JSON encode payload. Error code: 5 #55

Open devinfd opened 2 years ago

devinfd commented 2 years ago

Thank you for your work on this package! I've been fighting an issue for the past week that I can't find a resolution to. I don't know if this is an issue with this package or Laravel but I thought that I would start here.

I have a queued job that has an object (not a model) as constructor argument. The object has property that is a model with a coordinates (Point) attribute.

something like:

class Report
{
  $account = null;
  public function __construct(Model $account) {
    $this->account = $account
  }
}

$account = Account::first();
MakeReport::dispatch(new Report($account));

The coordinates attribute is properly casted on the Account model

protected $casts = [
    'coordinates' => Point::class
];

The problem is that the coordinates are not being serialized by laravel and I get the error: Illuminate\ Queue\ InvalidPayloadException Unable to JSON encode payload. Error code: 5

Any ideas? I'm starting to lose my mind on this.

PHP 8.1.10 Laravel 9.28.0

devinfd commented 2 years ago

I may have narrowed this down to a smaller issue. A model with a geospatial attribute passed to a job causes the Illuminate\ Queue\ InvalidPayloadException Unable to JSON encode payload. Error code: 5 error because it is binary data. When Laravel does json_encode on the queable object it fails.

https://laravel.com/docs/9.x/queues#handle-method-dependency-injection

Binary data, such as raw image contents, should be passed through the base64_encode function before being passed to a queued job. Otherwise, the job may not properly serialize to JSON when being placed on the queue.

MatanYadaev commented 2 years ago

@devinfd Hi Devin, I tried reproducing the issue you mentioned but didn't find anything. The geometry objects of this package are serializable and jsonable, I tend to believe that the issue you mention isn't related to this package.

Anyway, if you'll find something, please fork this repo and add a failing test so it will be easy to me to play with it and fix it.

devinfd commented 2 years ago

Yes and no. Laravel jobs (that use SerializesModels) that have a model constructor argument work because SerializesModels extracts the model identifier and relationships. That data alone is passed through json_encode in \Illuminate\Queue\Queue@createPayload. If however the job has a constructor argument that is not a model (but has a model as it's own property) then SerializesModels does not work and the entire object passes through json_encode. At no point is the geospatial binary data transformed into a MatanYadaev\EloquentSpatial object. json_encode does not trigger any magic methods to serialize or encode the binary geospatial data and therefore throws the Unable to JSON encode payload. Error code: 5 exception.

This issue was first described in 2015: https://github.com/laravel/framework/issues/11100 and various other issues, ex: https://github.com/michaeldyrynda/laravel-efficient-uuid/issues/22

This issue was solved in grimzy/laravel-mysql-spatial by overriding setRawAttributes https://github.com/grimzy/laravel-mysql-spatial/blob/2ca9f2f25cf10e3b4771881108ecc9c69317bf57/src/Eloquent/SpatialTrait.php#L102.

Just as a test, if I make the below change in my model then everything works. Model custom casts are a nice but also flawed.

/**
 * The list of attributes to cast.
 *
 * @var array
 */
protected $casts = [
    //'coordinates' => Point::class
];

public function setRawAttributes(array $attributes, $sync = false)
{
    $spatial_fields = ['coordinates'];

    foreach ($attributes as $attribute => &$value) {
        if (in_array($attribute, $spatial_fields) && is_string($value) && strlen($value) >= 13) {
            $value = \MatanYadaev\EloquentSpatial\Objects\Point::fromWkb($value);
        }
    }

    return parent::setRawAttributes($attributes, $sync);
}
devinfd commented 1 year ago

@MatanYadaev I would appreciate a response to the above comments.

MatanYadaev commented 1 year ago

@devinfd I agree Devin, custom casts are nice, but not perfect, unfortunately.

  1. Can you please help me to reproduce this issue? Can you fork this repo and add a failing test?
  2. Do you have a suggestion on how to fix it?
devinfd commented 1 year ago

I found a solution. The miscellaneous object with a model property needs to use the SerializesModels trait. PR: https://github.com/MatanYadaev/laravel-eloquent-spatial/pull/59 has tests that illistrate this. The only other thing I can think of is to add a comment in the docs about this requirement.

MatanYadaev commented 1 year ago

I see, you can simplify the test to this code:

  $testPlace = TestPlace::factory()->create([
    'point' => new Point(0, 180),
  ])->fresh();

  $json = json_encode([
    'serialized_test_place' => serialize($testPlace),
  ]);

  expect($json)->toBeFalse();
  expect(json_last_error())->toBe(5);

As you already noticed, fixing this issue will require migrating from custom casts to setRawAttributes. I'm not sure I want to do it at the moment. I'm happy you found a solution for now. If I'll see that this binary representation will produce more issues for this package users, I'll re-consider doing this change.

Thanks for bringing this to my attention.

MatanYadaev commented 1 year ago

About the comment in the docs - can you please explain what would you expect to be written in the docs?

devinfd commented 1 year ago

I don't think think this is a MatanYadaev/laravel-eloquent-spatial package issue. It's just a consequence of how MySql stores the data and how Laravel serializes objects. Kind-of an edge case that people have randomly experienced over the years. As for the docs; just a helpful note like:

NOTE: MySql stores spatial data as a Well-Known Binary (WKB) format. If you're queueing an object in a Laravel job containing a model with a spatial attribute, make sure you apply the "SerializesModels" trait to that object. Not doing so will lead to a JSON encode exception.

I found this random tweet that was helpful: https://twitter.com/realstevebauman/status/1517522880002772993

blackjak231 commented 1 year ago

@devinfd Thank you for this workaround. Making a Trait to include this instead the custom cast worked like a charm.

FYI: Including use SerializesModels on my models did not cut it, I'de have to include it in almost all my models as my "pointable" model is nested in many of them.

Not having the time to make a PR, i do hope @MatanYadaev or anyone else will have the time to fix this.

Yes and no. Laravel jobs (that use SerializesModels) that have a model constructor argument work because SerializesModels extracts the model identifier and relationships. That data alone is passed through json_encode in \Illuminate\Queue\Queue@createPayload. If however the job has a constructor argument that is not a model (but has a model as it's own property) then SerializesModels does not work and the entire object passes through json_encode. At no point is the geospatial binary data transformed into a MatanYadaev\EloquentSpatial object. json_encode does not trigger any magic methods to serialize or encode the binary geospatial data and therefore throws the Unable to JSON encode payload. Error code: 5 exception.

This issue was first described in 2015: laravel/framework#11100 and various other issues, ex: michaeldyrynda/laravel-efficient-uuid#22

This issue was solved in grimzy/laravel-mysql-spatial by overriding setRawAttributes https://github.com/grimzy/laravel-mysql-spatial/blob/2ca9f2f25cf10e3b4771881108ecc9c69317bf57/src/Eloquent/SpatialTrait.php#L102.

Just as a test, if I make the below change in my model then everything works. Model custom casts are a nice but also flawed.

/**
 * The list of attributes to cast.
 *
 * @var array
 */
protected $casts = [
    //'coordinates' => Point::class
];

public function setRawAttributes(array $attributes, $sync = false)
{
    $spatial_fields = ['coordinates'];

    foreach ($attributes as $attribute => &$value) {
        if (in_array($attribute, $spatial_fields) && is_string($value) && strlen($value) >= 13) {
            $value = \MatanYadaev\EloquentSpatial\Objects\Point::fromWkb($value);
        }
    }

    return parent::setRawAttributes($attributes, $sync);
}
localgituser commented 1 year ago

Hitting the same error :(. Using setRawAttributes solves this issue but then I get other errors when I use whereDistanceSphere function.

MatanYadaev commented 1 year ago

@localgituser Can you please fork this repo and push a commit with a failing test? I need a way to reproduce this issue.

Samuel-Webster commented 10 months ago

FWIW - I recently came across the same problem, but due to other requirements the above solutions didn't quite work for me. I still wanted to use the casts and all the other benefits this package comes with. Not sure if this will work in all situations, but works well for me.

In the end I found this approach to do the trick, it may be something that can be implemented, or help others if they get stuck. I've kept it simple for this example, but nothing to stop it being abstracted away, like the static retrieved/saving/etc hooks on the model or higher.

In short, this makes sure the "original" gets the same query expression as the binary "attribute", which means it can then be encoded properly in jobs/queues.

I also found that when running an artisan command, and other situations where the property has not been called on before you go to save, attach to a pivot table, or whatever else you are doing, you need to call the property with the cast on it first to get it to apply.

// Not *always* required, but safe if you forget. The cast needs to be invoked at some point.
$yourModel->coordinates = $yourModel->coordinates;

// Replaces the original binary (the part causing all the headaches) with the query expression.
$yourModel->syncOriginalAttribute('coordinates');

$yourModel->save();
// or...
$yourModel->places()->attach($placeId);
// or dispatching jobs, etc
d0m4te commented 9 months ago

@MatanYadaev A way to reproduce the issue: json_encode(serialize($modelWithSpatialAttributes)); --> Throws the discussed error.

The problem is, the Laravel Queue uses the scheme above to JSON encode object payloads (see https://github.com/laravel/framework/blob/10.x/src/Illuminate/Queue/Queue.php @ lines 106-->128-->157).

Playing of of @devinfd s suggestion combined with @Samuel-Webster s one could try to 'abuse' the casting in setRawAttributes without any problems which would occur while trying to save the model. I would propose this addition to the HasSpatial trait:

namespace MatanYadaev\EloquentSpatial\Traits;

use MatanYadaev\EloquentSpatial\Objects\Geometry;

trait HasSpatial {
    // ...

    public function setRawAttributes(array $attributes, $sync = false)
    {
        $result = parent::setRawAttributes($attributes, $sync);

        foreach ($attributes as $attribute => $value) {
            if ($value && is_string($value) && ! preg_match('//u', $value)) { // the string is binary
                // access the attribute to force conversion via attribute cast
                $spatialAttribute = $this->$attribute;

                // override attribute and original attribute to get rid of binary strings
                // (Those would lead to errors while JSON encoding a serialized version of the model.)
                if ($spatialAttribute instanceof Geometry) {
                    $this->attributes[$attribute] = $spatialAttribute;
                    $this->original[$attribute] = $spatialAttribute;
                }
            }
        }

        return $result;
    }
}
MatanYadaev commented 9 months ago

Hi @d0m4te, can you please submit a PR with tests?

simoMGH commented 8 months ago

The issue is still present after the revert of the PR #100 in V3.2.2. @MatanYadaev @d0m4te it's possible to reopen the issue since the encoding problem is still a thing?