spatie / laravel-webhook-client

Receive webhooks in Laravel apps
https://freek.dev/1383-sending-and-receiving-webhooks-in-laravel-apps
MIT License
1k stars 147 forks source link

Remove `SerializesModels` for performance gains #196

Closed mabdullahsari closed 1 year ago

mabdullahsari commented 1 year ago

Hi πŸ‘‹

This PR removes the SerializesModels trait from the base ProcessWebhookJob class.


Please allow me to explain the reasoning behind this change proposal. The documentation states (verbatim):

If your queued job accepts an Eloquent model in its constructor, only the identifier for the model will be serialized onto the queue. When the job is actually handled, the queue system will automatically re-retrieve the full model instance and its loaded relationships from the database. This approach to model serialization allows for much smaller job payloads to be sent to your queue driver.

While this behavior is fine for the vast majority of use cases out there, I think that free performance gains are left on the table by utilizing this trait within this particular context of webhooks. As you may already know, incoming webhooks are essentially events dispatched over Http, and by definition events are always immutable because they represent things that happened in the past. This immutable nature of events guarantees us that no changes to the serialized WebhookCall are going to occur by the time a queue worker picks up the job and starts processing it. Consequently, each processed job is no longer going to make an additional database query to retrieve the serialized model thus resulting in performance improvements.

I don't think that Job payload is going to be an issue, as most webhook payloads have a small footprint anyway.


This is definitely a breaking change, and I'm targeting main right now because there was no other option. I'm definitely glad to close and re-open the PR if a next or v4 branch is made.

Thanks πŸ™Œ

freekmurze commented 1 year ago

Thanks for explaining. I'm going to pass on this as in the vast majority of cases having the SerializesModels trait won't pose a real-world problem.

I'll take a look again when working on a new major version.